Prevent missing ActionLog entries to break the application#9502
Prevent missing ActionLog entries to break the application#9502ahukkanen merged 4 commits intodecidim:developfrom
Conversation
|
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! |
ahukkanen
left a comment
There was a problem hiding this comment.
This seems like a great improvement @microstudi, thanks a lot!
I believe this will also fix the issue #9423, right?
Could you add a spec for the added case at the ActionLog#visible_for? method?
I also left one refactoring idea below.
Pretty sure this solves that case, but there are cases not handled yet (I'm thinking on having content blocks refering to non-existing modules or the links between participatory process/assemblies, projects/budgets, meetings/proposals). |
|
Also ready @ahukkanen ! |
ahukkanen
left a comment
There was a problem hiding this comment.
Tested this and this solves the main described issue (the issue in the title) which is why I'll approve this.
But this won't solve the other described issue completely:
- Similarly, it is possible to have content created for different modules and, in a later stage, remove the module (imaging for example to create a Decidim with consultations, some content in it, and then you want to remove the "consultations" entirerly). This leads to a situation where some tables in the database contains references to a module that no longer exists and, when presented to the user, tries to instantiate a resource that no longer has a handler for it. I found that the ActionLog table has this problem (it is possible that this problem arises in other places).
The approach won't always work because when the gem definitions are loaded through global gems (in the case of Gem.loaded_specs) or through bundler directory or git installations, the modules will be defined when Decidim loads the version definitions for each gem as in:
The problem is in situations where these gems exist in the gem directory (installed through other projects or through gem install), in these situations the gem will be available in Gem.loaded_specs as it lists all the gems in the load paths. Similarly, if we install the Decidim gems through the git repository or in this repository's development_app, the gems that are not listed in the Gemfile will be still listed in Gem.loaded_specs because they will be in the load path.
You can also test this e.g. as follows:
- Comment out
gem "decidim", path: ".."in the development app - Add
gem "decidim-core", path: ".."instead - Run
bundle install - Try to start up the application
- See an exception
You can also inspect Gemfile.lock in that situation and you'll see a bunch of other Decidim gems there as well instead of just decidim-core. This happens when you bundle install Decidim from directory (i.e. the main repo development_app) or from git sources.
I was initially thinking we could solve this through Bundler instead of Gem.loaded_specs (to only list the gems in the bundle but not all gems in the load paths) but that won't work either because of the reason explained above.
This also means that #9423 will not be fixed by this issue. I'll continue investigation and see if I can find any way to solve that in all situations.
…ging * feature/redesign-main-footer: Reorder elements in main links of footer and define links and texts Define a cell for static_pages and topics configured to appear in footer Fix translation call Set fixed links in redesigned_main_legal partial Add FooterMenuPresenter to display menu items in footer Fix budgets seeds on non development apps (#9585) Return 404 when there isn't a valid component in program (#9576) Add missing queue close_meeting_reminder to sidekiq configuration (#9568) Make the HERE Map display in the currently selected language (#9552) Add help text for proposals' 'publish answers immediately' setting (#9549) Fix admin language selector with more than 4 locales (#9519) Fix publish event on official proposals (#9421) Prevent missing ActionLog entries to break the application (#9502) Add boilerplate structure to CHANGELOG (#9501) Add step-by-step instructions of the Crowdin releases process (#9555) Fix translated attributes field type change (#9547) Add `modifyList` option to the autocomplete element (#9548) Admin log filters (#9460) Improve the default gitignore files created by the generators (#9507)
🎩 What? Why?
This is work in progress but adding the PR in case anyone has comments on it.
This solves 2 problems that can appear In certain edge cases
Decidim.module_installed?method, but this does not take into account if a module is installed but not required (this can be done by adding into the Gemfile the linegem "decidim-consultations", require: falsefor instance. We want that method to return false in any situation where the module is not loaded for consistency.In summary, I think it is worth it to make Decidim as resilient as possible. This kind of changes also opens the door to create Decidim instances where some modules are initiated or no based on ENV vars (something that might be relevant for a future Decidim installer).
📌 Related Issues
Link your PR to an issue
Testing
Describe the best way to test or validate your PR.
📋 Checklist
🚨 Please review the guidelines for contributing to this repository.
docs/.📷 Screenshots
Please add screenshots of the changes you're proposing
