Add attachment enabled option to initiative types and initiatives#6036
Conversation
|
@carolromero you can test this on osp-decidim.dev.aspgems.com/initiatives |
|
Looks good to me, thanks @virgile-dev @ace |
|
@tramuntanal This is approved by product can you guys review it and see if we can merge ? Thanks in advance :) |
microstudi
left a comment
There was a problem hiding this comment.
@ace please, can you rebase develop here? thanks!
|
Hi @microstudi. Sure, I was 'waiting' for this, too many PRs around the same code. |
d9bddba to
0f95414
Compare
microstudi
left a comment
There was a problem hiding this comment.
Hi @ace nice work.
I have just a very slight change for extra safety!
| return unless permission_action.action == :add_attachment && | ||
| permission_action.subject == :initiative | ||
|
|
||
| toggle_allow(initiative_type.attachments_enabled?) |
There was a problem hiding this comment.
| toggle_allow(initiative_type.attachments_enabled?) | |
| toggle_allow(initiative_type&.attachments_enabled?) |
As initiative_type might be nil, should we be extra careful here?
There was a problem hiding this comment.
Hi @microstudi, thanks!
I'm not sure we need that, at the point we check that attachments permission the initiative_type is required. It can be nil only in the first step of the creation wizard and we check that on the third/fourth and on edit.
Do you think it is worth adding it just in case a future change may need it?
There was a problem hiding this comment.
Well, I'd put it, unless we are sure that initiative_type will never be nil. In which case:
def initiative_type
@initiative_type ||= context.fetch(:initiative_type, nil)
endShouldn't return a default nil value. Just for coherence. But you are right, it will probably never be a problem in real life.
There was a problem hiding this comment.
Ah, I see your point, you're right. That fetch is wrong. There should always be an initiative_type at that point. If there is not, then we have an error, there's no point in having a nil default. Thanks for pointing it out!
|
@tramuntanal this can be merged now ! Thanks in advance ! |
* develop: Include year in meetings card (decidim#6102) Add attachment enabled option to initiative types and initiatives (decidim#6036) Fix a flaky test in group profile conversations (decidim#6123) Add attachments to Initiatives (decidim#5844) Add initiatives export (decidim#6070) Improvements to conversations with more than one participant (decidim#6094) Elections module and election administration (decidim#6065) Separate forms in steps (decidim#6108) Add sorting by publishing date to initiatives (decidim#6016) Improve proposal preview: Use proposal card when previewing a proposal draft (decidim#6064) Newsletter templates fixes (decidim#6096) # Conflicts: # decidim-initiatives/app/models/decidim/initiative.rb # decidim-initiatives/spec/system/admin/initiative_types_controller_spec.rb
* feature/add_areas_to_initiatives: (30 commits) Adds areas to FO filters Fix lint issue Fixes rubocop issues Updates changelog Adds areas to initiatives Send notification when signature threshold reached (decidim#6098) Adds filter by initiative type to admin panel (decidim#6093) Require confirmation on exiting a survey mid-answering (decidim#6118) Information message when there isn't any Proposal (decidim#6063) Set email asset host dynamically (decidim#5888) Harmonizes the design of initiatives search in FO (decidim#6090) Include year in meetings card (decidim#6102) Add attachment enabled option to initiative types and initiatives (decidim#6036) Fix a flaky test in group profile conversations (decidim#6123) Add attachments to Initiatives (decidim#5844) Add initiatives export (decidim#6070) Improvements to conversations with more than one participant (decidim#6094) Elections module and election administration (decidim#6065) Separate forms in steps (decidim#6108) Add sorting by publishing date to initiatives (decidim#6016) ... # Conflicts: # decidim-initiatives/app/cells/decidim/initiatives/initiative_m_cell.rb # decidim-initiatives/app/commands/decidim/initiatives/admin/update_initiative.rb # decidim-initiatives/app/controllers/decidim/initiatives/initiatives_controller.rb # decidim-initiatives/app/forms/decidim/initiatives/admin/initiative_form.rb # decidim-initiatives/app/helpers/decidim/initiatives/application_helper.rb # decidim-initiatives/app/models/decidim/initiative.rb # decidim-initiatives/app/services/decidim/initiatives/initiative_search.rb # decidim-initiatives/app/views/decidim/initiatives/create_initiative/fill_data.html.erb # decidim-initiatives/app/views/decidim/initiatives/initiatives/_filters.html.erb # decidim-initiatives/app/views/decidim/initiatives/initiatives/_tags.html.erb # decidim-initiatives/config/locales/en.yml # decidim-initiatives/db/migrate/20200514085422_add_area_to_initiatives.rb # decidim-initiatives/db/migrate/20200514102631_add_area_enabled_option_to_initiatives.rb # decidim-initiatives/spec/forms/initiative_form_spec.rb # decidim-initiatives/spec/services/decidim/initiatives/initiative_search_spec.rb # decidim-initiatives/spec/shared/update_initiative_type_example.rb # decidim-initiatives/spec/system/admin/admin_manages_initiatives_spec.rb # decidim-initiatives/spec/system/admin/initiative_types_controller_spec.rb # decidim-initiatives/spec/system/filter_initiatives_spec.rb
|
@ace it looks like your PR was conflicting with this one #5844 #5844 allows user to add a joint file in the iniative creation wizzard There were merged about at the same time. It looks like that you anticipated this but it's not working as expected. Testing your feature on the udpated test instance it appears that it messed with your config flag to enable and disable the possibility to add joint files. **Bug description : **
|
|
@virgile-dev That PR you linked that I made to integrate the attachments permissions was never merged into the branch on #5844, so my fix is not added. |
🎩 What? Why?
Allow admins to configure whether initiatives for a type will allow attachments or not.
📌 Related Issues
📋 Subtasks
CHANGELOGentry