Remove notifications if article is unpublished #3362
Conversation
| @@ -268,6 +267,15 @@ def article_params_json | |||
| params.require(:article).permit(allowed_params) | |||
| end | |||
|
|
|||
| def handle_notifications(updated) | |||
| if updated && @article.published && @article.saved_changes["published"] == [false, true] | |||
There was a problem hiding this comment.
Wouldn't the following be enough?
| if updated && @article.published && @article.saved_changes["published"] == [false, true] | |
| if updated && @article.saved_changes["published"] == [false, true] |
There was a problem hiding this comment.
Yeah probably. I decided to keep the previous code and think it over. It's probably fine to change it though.
| Notification.send_to_followers(@article, "Published") | ||
| elsif @article.saved_changes["published"] == [true, false] | ||
| Notification.remove_all_without_delay(notifiable_id: @article.id, notifiable_type: "Article", action: "Published") | ||
| Notification.remove_all(notifiable_id: @article.id, notifiable_type: "Article", action: "Reaction") |
There was a problem hiding this comment.
Should we consider also removing article's comment notification? I could be overthinking this.
pr-triage
bot
added
PR: reviewed-approved
| if updated && @article.published && @article.saved_changes["published"] == [false, true] | ||
| Notification.send_to_followers(@article, "Published") | ||
| elsif @article.saved_changes["published"] == [true, false] | ||
| Notification.remove_all_without_delay(notifiable_id: @article.id, notifiable_type: "Article", action: "Published") |
There was a problem hiding this comment.
It's not clear to me why removing "Published" notifications is synchronous and removing "Reaction" notifications is async 🤔
There was a problem hiding this comment.
@lightalloy The publish notifications contain the data of the article, and is sent to the followers. IMO those should be removed immediately since that data is revoked. The reaction notifications only belong to the author so I think it's okay of those are removed async.
I think based on how we handle reaction notifications though there's only one reaction notification for the article. So performance-wise it probably doesn't matter all that much. Ah never mind, I'm wrong about that, oops 🙈
pr-triage
bot
added
PR: merged
Fix for missing post_reaction_notifications on organizations Allow full links for cache buster tool (forem#3378) Rescue possible SSLError when fetching a podcast (forem#3374) Add functionality to remove and recover identity (forem#3377) * Lint some quotation marks * Add BackupData table * Add identity and removal functionality * Test additional functionality * Remove dependent destroy for backup data * Add auth_data_dump column * Add challenge to reserved words * Add more shoulda matchers Self-serve sponsorship options (forem#3371) * WIP - Initial work on sponsorship landing page * Add initial partnership page Remove notifications if article is unpublished (forem#3362) * Add tests for article notifications * Remove notifications if unpublished and refactor Moved creating a new podcast to a separate ActiveJob forem#2952 (forem#3373) * Podcasts reachable status forem#2952 * Specs for podcasts statuses * Moved podcast episode create to a separate ActiveJob * Use RssItemData wrapper in GetEpisode for consistency * Reorganize services/podcasts Escape comment.title to fix failing spec (forem#3385)
What type of PR is this? (check all applicable)
Description
This fixes an issue where some notifications will linger if an article is published and then unpublished.