From 16f4dfd3e9dd5b1619734333dfeb2cf37607295d Mon Sep 17 00:00:00 2001 From: Bowrna Date: Thu, 19 Sep 2024 10:56:56 +0530 Subject: [PATCH] Fix incorrect bulk blocklisting behaviour (#2041). Fixes #1841 --- cmd/subscribers.go | 23 ++++++++++++----------- frontend/src/views/Subscribers.vue | 4 +++- internal/core/subscribers.go | 8 ++++---- internal/core/subscriptions.go | 12 ++++++------ models/queries.go | 12 +++++------- queries.sql | 7 ++++--- 6 files changed, 34 insertions(+), 32 deletions(-) diff --git a/cmd/subscribers.go b/cmd/subscribers.go index 353387b7..46aad2db 100644 --- a/cmd/subscribers.go +++ b/cmd/subscribers.go @@ -22,12 +22,13 @@ const ( // subQueryReq is a "catch all" struct for reading various // subscriber related requests. type subQueryReq struct { - Query string `json:"query"` - ListIDs []int `json:"list_ids"` - TargetListIDs []int `json:"target_list_ids"` - SubscriberIDs []int `json:"ids"` - Action string `json:"action"` - Status string `json:"status"` + Query string `json:"query"` + ListIDs []int `json:"list_ids"` + TargetListIDs []int `json:"target_list_ids"` + SubscriberIDs []int `json:"ids"` + Action string `json:"action"` + Status string `json:"status"` + SubscriptionStatus string `json:"subscription_status"` } // subProfileData represents a subscriber's collated data in JSON @@ -415,7 +416,7 @@ func handleDeleteSubscribersByQuery(c echo.Context) error { return err } - if err := app.core.DeleteSubscribersByQuery(req.Query, req.ListIDs); err != nil { + if err := app.core.DeleteSubscribersByQuery(req.Query, req.ListIDs, req.SubscriptionStatus); err != nil { return err } @@ -434,7 +435,7 @@ func handleBlocklistSubscribersByQuery(c echo.Context) error { return err } - if err := app.core.BlocklistSubscribersByQuery(req.Query, req.ListIDs); err != nil { + if err := app.core.BlocklistSubscribersByQuery(req.Query, req.ListIDs, req.SubscriptionStatus); err != nil { return err } @@ -461,11 +462,11 @@ func handleManageSubscriberListsByQuery(c echo.Context) error { var err error switch req.Action { case "add": - err = app.core.AddSubscriptionsByQuery(req.Query, req.ListIDs, req.TargetListIDs, req.Status) + err = app.core.AddSubscriptionsByQuery(req.Query, req.ListIDs, req.TargetListIDs, req.Status, req.SubscriptionStatus) case "remove": - err = app.core.DeleteSubscriptionsByQuery(req.Query, req.ListIDs, req.TargetListIDs) + err = app.core.DeleteSubscriptionsByQuery(req.Query, req.ListIDs, req.TargetListIDs, req.SubscriptionStatus) case "unsubscribe": - err = app.core.UnsubscribeListsByQuery(req.Query, req.ListIDs, req.TargetListIDs) + err = app.core.UnsubscribeListsByQuery(req.Query, req.ListIDs, req.TargetListIDs, req.SubscriptionStatus) default: return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("subscribers.invalidAction")) } diff --git a/frontend/src/views/Subscribers.vue b/frontend/src/views/Subscribers.vue index f05e0559..1e13168e 100644 --- a/frontend/src/views/Subscribers.vue +++ b/frontend/src/views/Subscribers.vue @@ -375,6 +375,7 @@ export default Vue.extend({ this.$api.blocklistSubscribersByQuery({ query: this.queryParams.queryExp, list_ids: this.queryParams.listID ? [this.queryParams.listID] : null, + subscription_status: this.queryParams.subStatus, }).then(() => this.querySubscribers()); }; } @@ -426,6 +427,7 @@ export default Vue.extend({ this.$api.deleteSubscribersByQuery({ query: this.queryParams.queryExp, list_ids: this.queryParams.listID ? [this.queryParams.listID] : null, + subscription_status: this.queryParams.subStatus, }).then(() => { this.querySubscribers(); @@ -460,6 +462,7 @@ export default Vue.extend({ } else { // 'All' is selected, perform by query. data.query = this.queryParams.queryExp; + data.subscription_status = this.queryParams.subStatus; fn = this.$api.addSubscribersToListsByQuery; } @@ -503,7 +506,6 @@ export default Vue.extend({ // Get subscribers on load. this.querySubscribers(); } - if (this.$route.query.subscription_status) { this.queryParams.subStatus = this.$route.query.subscription_status; } diff --git a/internal/core/subscribers.go b/internal/core/subscribers.go index f2159170..64c0b550 100644 --- a/internal/core/subscribers.go +++ b/internal/core/subscribers.go @@ -394,8 +394,8 @@ func (c *Core) BlocklistSubscribers(subIDs []int) error { } // BlocklistSubscribersByQuery blocklists the given list of subscribers. -func (c *Core) BlocklistSubscribersByQuery(query string, listIDs []int) error { - if err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.BlocklistSubscribersByQuery, listIDs, c.db); err != nil { +func (c *Core) BlocklistSubscribersByQuery(query string, listIDs []int, subStatus string) error { + if err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.BlocklistSubscribersByQuery, listIDs, c.db, subStatus); err != nil { c.log.Printf("error blocklisting subscribers: %v", err) return echo.NewHTTPError(http.StatusInternalServerError, c.i18n.Ts("subscribers.errorBlocklisting", "error", pqErrMsg(err))) @@ -423,8 +423,8 @@ func (c *Core) DeleteSubscribers(subIDs []int, subUUIDs []string) error { } // DeleteSubscribersByQuery deletes subscribers by a given arbitrary query expression. -func (c *Core) DeleteSubscribersByQuery(query string, listIDs []int) error { - err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.DeleteSubscribersByQuery, listIDs, c.db) +func (c *Core) DeleteSubscribersByQuery(query string, listIDs []int, subStatus string) error { + err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.DeleteSubscribersByQuery, listIDs, c.db, subStatus) if err != nil { c.log.Printf("error deleting subscribers: %v", err) return echo.NewHTTPError(http.StatusInternalServerError, diff --git a/internal/core/subscriptions.go b/internal/core/subscriptions.go index c5a78a90..7ea0fa11 100644 --- a/internal/core/subscriptions.go +++ b/internal/core/subscriptions.go @@ -35,12 +35,12 @@ func (c *Core) AddSubscriptions(subIDs, listIDs []int, status string) error { // AddSubscriptionsByQuery adds list subscriptions to subscribers by a given arbitrary query expression. // sourceListIDs is the list of list IDs to filter the subscriber query with. -func (c *Core) AddSubscriptionsByQuery(query string, sourceListIDs, targetListIDs []int, status string) error { +func (c *Core) AddSubscriptionsByQuery(query string, sourceListIDs, targetListIDs []int, status string, subStatus string) error { if sourceListIDs == nil { sourceListIDs = []int{} } - err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.AddSubscribersToListsByQuery, sourceListIDs, c.db, pq.Array(targetListIDs), status) + err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.AddSubscribersToListsByQuery, sourceListIDs, c.db, subStatus, pq.Array(targetListIDs), status) if err != nil { c.log.Printf("error adding subscriptions by query: %v", err) return echo.NewHTTPError(http.StatusInternalServerError, @@ -64,12 +64,12 @@ func (c *Core) DeleteSubscriptions(subIDs, listIDs []int) error { // DeleteSubscriptionsByQuery deletes list subscriptions from subscribers by a given arbitrary query expression. // sourceListIDs is the list of list IDs to filter the subscriber query with. -func (c *Core) DeleteSubscriptionsByQuery(query string, sourceListIDs, targetListIDs []int) error { +func (c *Core) DeleteSubscriptionsByQuery(query string, sourceListIDs, targetListIDs []int, subStatus string) error { if sourceListIDs == nil { sourceListIDs = []int{} } - err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.DeleteSubscriptionsByQuery, sourceListIDs, c.db, pq.Array(targetListIDs)) + err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.DeleteSubscriptionsByQuery, sourceListIDs, c.db, subStatus, pq.Array(targetListIDs)) if err != nil { c.log.Printf("error deleting subscriptions by query: %v", err) return echo.NewHTTPError(http.StatusInternalServerError, @@ -92,12 +92,12 @@ func (c *Core) UnsubscribeLists(subIDs, listIDs []int, listUUIDs []string) error // UnsubscribeListsByQuery sets list subscriptions to 'unsubscribed' by a given arbitrary query expression. // sourceListIDs is the list of list IDs to filter the subscriber query with. -func (c *Core) UnsubscribeListsByQuery(query string, sourceListIDs, targetListIDs []int) error { +func (c *Core) UnsubscribeListsByQuery(query string, sourceListIDs, targetListIDs []int, subStatus string) error { if sourceListIDs == nil { sourceListIDs = []int{} } - err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.UnsubscribeSubscribersFromListsByQuery, sourceListIDs, c.db, pq.Array(targetListIDs)) + err := c.q.ExecSubQueryTpl(sanitizeSQLExp(query), c.q.UnsubscribeSubscribersFromListsByQuery, sourceListIDs, c.db, subStatus, pq.Array(targetListIDs)) if err != nil { c.log.Printf("error unsubscribing from lists by query: %v", err) return echo.NewHTTPError(http.StatusInternalServerError, diff --git a/models/queries.go b/models/queries.go index 7a3a783a..9e978e4d 100644 --- a/models/queries.go +++ b/models/queries.go @@ -114,7 +114,7 @@ type Queries struct { // out of it using the raw `query-subscribers-template` query template. // While doing this, a readonly transaction is created and the query is // dry run on it to ensure that it is indeed readonly. -func (q *Queries) CompileSubscriberQueryTpl(exp string, db *sqlx.DB) (string, error) { +func (q *Queries) CompileSubscriberQueryTpl(exp string, db *sqlx.DB, subStatus string) (string, error) { tx, err := db.BeginTxx(context.Background(), &sql.TxOptions{ReadOnly: true}) if err != nil { return "", err @@ -126,19 +126,18 @@ func (q *Queries) CompileSubscriberQueryTpl(exp string, db *sqlx.DB) (string, er exp = " AND " + exp } stmt := fmt.Sprintf(q.QuerySubscribersTpl, exp) - if _, err := tx.Exec(stmt, true, pq.Int64Array{}); err != nil { + if _, err := tx.Exec(stmt, true, pq.Int64Array{}, subStatus); err != nil { return "", err } - return stmt, nil } // compileSubscriberQueryTpl takes an arbitrary WHERE expressions and a subscriber // query template that depends on the filter (eg: delete by query, blocklist by query etc.) // combines and executes them. -func (q *Queries) ExecSubQueryTpl(exp, tpl string, listIDs []int, db *sqlx.DB, args ...interface{}) error { +func (q *Queries) ExecSubQueryTpl(exp, tpl string, listIDs []int, db *sqlx.DB, subStatus string, args ...interface{}) error { // Perform a dry run. - filterExp, err := q.CompileSubscriberQueryTpl(exp, db) + filterExp, err := q.CompileSubscriberQueryTpl(exp, db, subStatus) if err != nil { return err } @@ -148,10 +147,9 @@ func (q *Queries) ExecSubQueryTpl(exp, tpl string, listIDs []int, db *sqlx.DB, a } // First argument is the boolean indicating if the query is a dry run. - a := append([]interface{}{false, pq.Array(listIDs)}, args...) + a := append([]interface{}{false, pq.Array(listIDs), subStatus}, args...) if _, err := db.Exec(fmt.Sprintf(tpl, filterExp), a...); err != nil { return err } - return nil } diff --git a/queries.sql b/queries.sql index 0df08327..7489afea 100644 --- a/queries.sql +++ b/queries.sql @@ -370,6 +370,7 @@ ON ( -- Optional list filtering. (CASE WHEN CARDINALITY($2::INT[]) > 0 THEN true ELSE false END) AND subscriber_lists.subscriber_id = subscribers.id + AND ($3 = '' OR subscriber_lists.status = $3::subscription_status) ) WHERE subscriber_lists.list_id = ALL($2::INT[]) %s LIMIT (CASE WHEN $1 THEN 1 END) @@ -393,20 +394,20 @@ UPDATE subscriber_lists SET status='unsubscribed', updated_at=NOW() -- raw: true WITH subs AS (%s) INSERT INTO subscriber_lists (subscriber_id, list_id, status) - (SELECT a, b, (CASE WHEN $4 != '' THEN $4::subscription_status ELSE 'unconfirmed' END) FROM UNNEST(ARRAY(SELECT id FROM subs)) a, UNNEST($3::INT[]) b) + (SELECT a, b, (CASE WHEN $5 != '' THEN $5::subscription_status ELSE 'unconfirmed' END) FROM UNNEST(ARRAY(SELECT id FROM subs)) a, UNNEST($4::INT[]) b) ON CONFLICT (subscriber_id, list_id) DO NOTHING; -- name: delete-subscriptions-by-query -- raw: true WITH subs AS (%s) DELETE FROM subscriber_lists - WHERE (subscriber_id, list_id) = ANY(SELECT a, b FROM UNNEST(ARRAY(SELECT id FROM subs)) a, UNNEST($3::INT[]) b); + WHERE (subscriber_id, list_id) = ANY(SELECT a, b FROM UNNEST(ARRAY(SELECT id FROM subs)) a, UNNEST($4::INT[]) b); -- name: unsubscribe-subscribers-from-lists-by-query -- raw: true WITH subs AS (%s) UPDATE subscriber_lists SET status='unsubscribed', updated_at=NOW() - WHERE (subscriber_id, list_id) = ANY(SELECT a, b FROM UNNEST(ARRAY(SELECT id FROM subs)) a, UNNEST($3::INT[]) b); + WHERE (subscriber_id, list_id) = ANY(SELECT a, b FROM UNNEST(ARRAY(SELECT id FROM subs)) a, UNNEST($4::INT[]) b); -- lists