Skip to content

Fix broken notifications page due to multi-budget changes#6815

Merged
tramuntanal merged 2 commits intodecidim:developfrom
mainio:fix/multibudget-broken-notifications
Nov 6, 2020
Merged

Fix broken notifications page due to multi-budget changes#6815
tramuntanal merged 2 commits intodecidim:developfrom
mainio:fix/multibudget-broken-notifications

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Nov 6, 2020

🎩 What? Why?

When notifications are created for the budgeting project comments, the notification pages are broken.

The following exception will be visible in the logs:

ActionView::Template::Error:
  undefined method `project_path' for #<ActionDispatch::Routing::RoutesProxy:0x000055b806a6f248>

# /.../decidim/decidim-core/lib/decidim/engine_router.rb:47:in `method_missing'
# /.../decidim/decidim-core/app/presenters/decidim/resource_locator_presenter.rb:101:in `member_route'
# /.../decidim/decidim-core/app/presenters/decidim/resource_locator_presenter.rb:20:in `path'
# /.../decidim/decidim-comments/app/events/decidim/comments/comment_event.rb:13:in `resource_path'

This is because the resource locator does not automatically work for the resources with "polymorphic" routes, e.g. in this case the budgeting project which is under a budget (and therefore the route helper is budget_project_path instead).

📌 Related Issues

The same issue applied to the activity feed items related to the budgeting component and this was fixed by @agustibr at this commit:
5d6cf9a

This PR applies the same fix to the notifications.

Testing

  • Create a Decidim instance with the seed content
  • Login as admin
  • Go to a participatory process with a budgeting component
  • Follow the participatory process
  • Open another window (incognito) and login as "user@example.org"
  • Go to the same participatory space which you just followed as admin
  • Go to budgets
  • Open one of the projects
  • Leave a comment in that project
  • Now as the admin user, refresh the page, you should have one notification
  • Go to the notifications view
  • See an exception

📋 Checklist

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

@tramuntanal tramuntanal merged commit c8eb9b3 into decidim:develop Nov 6, 2020
ahukkanen added a commit to mainio/decidim that referenced this pull request Nov 6, 2020
* Fix event paths/urls for resources with polymorphic paths

* Add a test for the budgeting project comment notifications
@tramuntanal
Copy link
Copy Markdown
Contributor

Thanks @ahukkanen can you backport to 0.23 please?

@ahukkanen ahukkanen deleted the fix/multibudget-broken-notifications branch November 6, 2020 15:25
@ahukkanen
Copy link
Copy Markdown
Contributor Author

Yes, it's already building at #6820 @tramuntanal

tramuntanal added a commit that referenced this pull request Nov 9, 2020
…to 0.23 (#6820)

* Fix broken notifications page due to multi-budget changes (#6815)

* Fix event paths/urls for resources with polymorphic paths

* Add a test for the budgeting project comment notifications

* Ignore no-emphasis-as-header mdl rule

Co-authored-by: Oliver Valls <199462+tramuntanal@users.noreply.github.com>
edgarlatorre pushed a commit that referenced this pull request Nov 11, 2020
* Fix event paths/urls for resources with polymorphic paths

* Add a test for the budgeting project comment notifications
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants