Refactor modules mounting routes by alecslupu · Pull Request #13294 · decidim/decidim

alecslupu · 2024-08-19T20:03:52Z

🎩 What? Why?

While working on #13267, i have noticed that some of the specs have started to fail due to some wrong routes being generated. Among those routes, are the admin templates, and admin participatory space. Upon a closer investigation, i have seen that the routes are being correctly generated in console, but for some reason they are not being correctly generated in the web interface.

The edit admin participatory space route was generated without the /admin namespace when called from the user interface, yet it was correctly generated when was called from admin interface.

Investigating rails routing data, i came across: rails/rails#51933 and rails/rails#45719 which may be the culprits of this issue.

Trying the fix suggested in rails/rails#51933 with an override, i got even more specs that were failing.
When i have looked at the changes suggested by rails/rails#45719, I could not find the config setting that would have allowed me to properly set the data at engine level.

In order to be able to keep the functionality, i had attempted to define some helper methods to redefine the decidim_admin_assemblies engine to be specially used for that button, yet it still failed. As an attempt to fix the issue, i have tried also the changes in this PR, and worked flawlessly.

I am opening a standalone PR, as this change could be elaborated more. While this PR changes only the admin routes :

decidim/decidim-admin/config/routes.rb

Lines 18 to 20 in ccfbe5d

    
           Decidim.participatory_space_manifests.each do |manifest| 
        
             mount manifest.context(:admin).engine, at: "/", as: "decidim_admin_#{manifest.name}" 
        
           end

and also

decidim/decidim-admin/config/routes.rb

Line 42 in ccfbe5d

    
           mount Decidim::Templates::AdminEngine, at: "/templates", as: "decidim_admin_templates" if Decidim.module_installed?(:templates)

An open question regarding the consistency still remains for verification engines mounted in admin, namely :

decidim/decidim-admin/config/routes.rb

Lines 38 to 40 in ccfbe5d

    
           Decidim.authorization_admin_engines.each do |manifest| 
        
             mount manifest.admin_engine, at: "/#{manifest.name}", as: "decidim_admin_#{manifest.name}" 
        
           end

And, also, for consistency reasons I would perform the same kind of changes for the frontend part.

📌 Related Issues

Link your PR to an issue

Related to Update Rails to v7.1 #13267

Testing

All the test suite should be green
Booting any application using this patch should still work as before.

♥️ Thank you!

github-actions

This pull request does not contain a valid label. Please add one of the following labels: ['type: feature', 'type: change', 'type: fix', 'type: removal', 'target: developer-experience', 'type: internal']

The merge-base changed after approval.

andreslucena · 2024-09-18T10:29:51Z

As we discussed in the meeting, I don't have any alternative way of doing this.

I agree with what you mention about consistency, the routes should be all the similar that we can between the different kind of modules (being for participants or admins).

Apart from what you mentioned and added in the PR, I'd also add some Releases Notes in 5. Changes in APIs, so developers can know why and how do this change in their modules.

RELEASE_NOTES.md

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

andreslucena · 2025-01-09T09:01:16Z

@alecslupu can you solve the git conflicts here 🙏🏽 ?

…-routes

…into chore/patch-routes

alecslupu · 2025-01-09T16:40:38Z

@alecslupu can you solve the git conflicts here 🙏🏽 ?

Done

andreslucena

I know this PR wasn't easy, but congratulations! Everything seems to work like a charm.

* develop: (27 commits) WCAG navigation submenu (#13796) Update gem dependencies (part 3) (#13849) Prevent notifications for deleted users (#13812) Retries NPM installation a couple times to prevent network timeouts (#13831) Update gem dependencies (part 2) (#13839) Enhance signature pdf export (#13778) Fix HexaPDF dependency (#13834) Fix flaky spec in authentication (#13827) Merge upload field for documents and image on proposal admin form (#13735) Update gem dependencies (#13835) Upgrade erb_lint to 0.8.0 (#13833) Fix flaky spec in geocoder (#13820) Refactor modules mounting routes (#13294) Upgrade check-spelling action (#13825) Add missing images in the custom registration emails from meetings (#13632) Add missing translations (#13793) Fix proposal map performance with hundreds of markers (#13798) Create multiple surveys within same Survey component (#13420) Accountability bulk actions (#13730) Improve UI for sorting options on comments (#13670) ...

* Refactor decidim mounting routes option * Fix failing specs * Apply review recommendation * Run lint on release notes * Apply suggestions from code review Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Run linters --------- Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

probot-autolabeler bot added module: admin module: assemblies module: conferences module: initiatives module: participatory processes module: templates labels Aug 19, 2024

github-actions bot requested changes Aug 19, 2024

View reviewed changes

alecslupu added the type: change PRs that implement a change for an existing feature label Aug 19, 2024

github-actions bot previously approved these changes Aug 19, 2024

View reviewed changes

alecslupu added this to the 0.30.0 milestone Aug 19, 2024

alecslupu mentioned this pull request Aug 19, 2024

Update Rails to v7.1 #13267

Merged

alecslupu dismissed github-actions[bot]’s stale review via 55a7e36 September 9, 2024 21:06

probot-autolabeler bot added configuration dependencies Pull requests that update a dependency file or issues that talk about updating dependencies javascript Pull requests that update Javascript code module: accountability labels Sep 9, 2024

github-actions bot previously approved these changes Sep 9, 2024

View reviewed changes

alecslupu force-pushed the chore/patch-routes branch from 55a7e36 to 01d3a5f Compare September 9, 2024 21:07

github-actions bot previously approved these changes Sep 9, 2024

View reviewed changes

alecslupu dismissed github-actions[bot]’s stale review via 9646079 September 29, 2024 16:26

github-actions bot previously approved these changes Sep 29, 2024

View reviewed changes

alecslupu dismissed github-actions[bot]’s stale review via 811be37 September 29, 2024 16:32

github-actions bot previously approved these changes Sep 29, 2024

View reviewed changes

alecslupu dismissed github-actions[bot]’s stale review via 46918e1 September 29, 2024 17:17

github-actions bot previously approved these changes Sep 29, 2024

View reviewed changes

alecslupu marked this pull request as ready for review September 29, 2024 18:36

alecslupu requested a review from andreslucena September 29, 2024 18:36

alecslupu assigned andreslucena Sep 29, 2024

alecslupu dismissed github-actions[bot]’s stale review via 8713754 January 7, 2025 10:28

github-actions bot previously approved these changes Jan 7, 2025

View reviewed changes

Run lint on release notes

1d10e9b

alecslupu dismissed github-actions[bot]’s stale review via 1d10e9b January 7, 2025 10:48

github-actions bot previously approved these changes Jan 7, 2025

View reviewed changes

alecslupu requested a review from andreslucena January 7, 2025 11:37

andreslucena reviewed Jan 8, 2025

View reviewed changes

RELEASE_NOTES.md Outdated Show resolved Hide resolved

andreslucena reviewed Jan 8, 2025

View reviewed changes

RELEASE_NOTES.md Outdated Show resolved Hide resolved

andreslucena reviewed Jan 8, 2025

View reviewed changes

RELEASE_NOTES.md Outdated Show resolved Hide resolved

andreslucena changed the title ~~Refactor decidim mounting routes option~~ Refactor modules mounting routes option Jan 8, 2025

andreslucena changed the title ~~Refactor modules mounting routes option~~ Refactor modules mounting routes Jan 8, 2025

Apply suggestions from code review

b0dd691

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

alecslupu dismissed github-actions[bot]’s stale review via b0dd691 January 8, 2025 10:08

github-actions bot previously approved these changes Jan 8, 2025

View reviewed changes

alecslupu requested a review from andreslucena January 8, 2025 10:10

Run linters

72a99f7

alecslupu dismissed github-actions[bot]’s stale review via 72a99f7 January 8, 2025 10:15

Merge branch 'develop' into chore/patch-routes

35be010

github-actions bot previously approved these changes Jan 8, 2025

View reviewed changes

alecslupu added 2 commits January 9, 2025 18:36

Merge branch 'develop' of github.com:decidim/decidim into chore/patch…

281b45b

…-routes

Merge branch 'chore/patch-routes' of github.com:tremend-cofe/decidim …

807ac41

…into chore/patch-routes

alecslupu dismissed github-actions[bot]’s stale review via 807ac41 January 9, 2025 16:36

github-actions bot approved these changes Jan 9, 2025

View reviewed changes

andreslucena approved these changes Jan 10, 2025

View reviewed changes

andreslucena merged commit 6e68f40 into decidim:develop Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor modules mounting routes#13294

Refactor modules mounting routes#13294
andreslucena merged 10 commits intodecidim:developfrom
i-need-another-coffee:chore/patch-routes

alecslupu commented Aug 19, 2024 •

edited

Loading

Uh oh!

github-actions bot left a comment

Uh oh!

andreslucena commented Sep 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreslucena commented Jan 9, 2025

Uh oh!

alecslupu commented Jan 9, 2025

Uh oh!

andreslucena left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	Decidim.participatory_space_manifests.each do \|manifest\|
	mount manifest.context(:admin).engine, at: "/", as: "decidim_admin_#{manifest.name}"
	end

	Decidim.authorization_admin_engines.each do \|manifest\|
	mount manifest.admin_engine, at: "/#{manifest.name}", as: "decidim_admin_#{manifest.name}"
	end

Uh oh!

Conversation

alecslupu commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

Testing

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

andreslucena commented Sep 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreslucena commented Jan 9, 2025

Uh oh!

alecslupu commented Jan 9, 2025

Uh oh!

andreslucena left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alecslupu commented Aug 19, 2024 •

edited

Loading