Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix notifications where resources are missing #9183

Merged
merged 3 commits into from Apr 29, 2022

Conversation

microstudi
Copy link
Contributor

@microstudi microstudi commented Apr 21, 2022

🎩 What? Why?

It is pretty common for admins to experiments different configurations that generate notifications. For instance create a step, activate it, deactivated, delete it.
This kind of processes tend to leave some notifications to some users with orphan resources. The current cell that renders a notification does not has a guard clause to skip or show and alternative title for the notification when this happens, resulting in a frustrating 500 error that prevents access to the notification page.

This PR simply shows a message (we can change that @andreslucena) when a resource is missing for whatever reason. At least this way the user can simply discard the notification.

Testing

Create a process, create some steps in it, make sure you follow the process. Activate/deactivate some step and then delete the step. Go to notifications page, you should see a "missing event" message.

Note: this should be backported

♥️ Thank you!

@request-info
Copy link

request-info bot commented Apr 21, 2022

It seems like you didn't give us much information about what you're trying to do here. We would appreciate it if you could provide us with more info about this issue/PR!

@microstudi microstudi changed the title fix notifications where resources are missing Fix notifications where resources are missing Apr 21, 2022
@andreslucena andreslucena added module: core type: fix PRs that implement a fix for a bug labels Apr 22, 2022
@andreslucena
Copy link
Member

Thanks for the PR @microstudi!
We're thinking in the best course of action with @carolromero
We'll let you know when we have a clear strategy

@andreslucena andreslucena added this to Pending review from Product in Maintainers via automation Apr 22, 2022
@microstudi
Copy link
Contributor Author

Thanks for the PR @microstudi! We're thinking in the best course of action with @carolromero We'll let you know when we have a clear strategy

Ok, but take into account that it is extremely difficult to prevent all the situations that can leave "broken" notification. This acts as a guard clause.
If you are thinking on simply not showing broken notifications, this can be more inefficient due the polymorphic nature of the resources associated.
This approach will simply let users "clean" these notification over time.

@andreslucena
Copy link
Member

If you are thinking on simply not showing broken notifications, this can be more inefficient due the polymorphic nature of the resources associated.

Yesterday we thought about that but we realized that this also works on other scenarios, and it'd be a bit weird for the user. For instance, what happens if you receive a notification by email, go to see it in your /notificaitons page, and can't find it? With a message at least you can see that "something" has happened.

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@microstudi
Copy link
Contributor Author

Yesterday we thought about that but we realized that this also works on other scenarios, and it'd be a bit weird for the user. For instance, what happens if you receive a notification by email, go to see it in your /notificaitons page, and can't find it? With a message at least you can see that "something" has happened.

absolutely right

@andreslucena
Copy link
Member

Failing related spec, we'll need to change it too. Can you take it a look @microstudi? Thanks

@microstudi
Copy link
Contributor Author

This should be ready @ahukkanen

@andreslucena andreslucena moved this from Pending review from Product to To Review by Core in Maintainers Apr 28, 2022
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@ahukkanen ahukkanen merged commit 7133211 into develop Apr 29, 2022
Maintainers automation moved this from To Review by Core to Done Apr 29, 2022
@ahukkanen ahukkanen deleted the fix/missing-resource-notifications branch April 29, 2022 13:55
andreslucena added a commit that referenced this pull request May 6, 2022
* fix notifications where resources are missing

* Update decidim-core/config/locales/en.yml

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

* Update notification_cell_spec.rb

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
ahukkanen pushed a commit that referenced this pull request May 10, 2022
* fix notifications where resources are missing

* Update decidim-core/config/locales/en.yml

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

* Update notification_cell_spec.rb

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

Co-authored-by: Ivan Vergés <ivan@platoniq.net>
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core type: fix PRs that implement a fix for a bug
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants