Add scope to proposals import in budgets#6525
Conversation
agustibr
left a comment
There was a problem hiding this comment.
😀 Good work!
Could you add a test example for an out-of-scope import?
decidim-budgets/app/commands/decidim/budgets/admin/import_proposals_to_budgets.rb
Outdated
Show resolved
Hide resolved
agustibr
left a comment
There was a problem hiding this comment.
Good work! 😁
Just one more comment, I would rename decidim_scope_id to scope_id so that the field translates correctly in the ProjectImportProposalsForm and in the view where the scope_picker is rendered and thus it is not necessary to translate this field again, as now renders "Decidim scope" and it should be "Scope".
decidim-budgets/app/views/decidim/budgets/admin/proposals_imports/new.html.erb
Outdated
Show resolved
Hide resolved
tramuntanal
left a comment
There was a problem hiding this comment.
Sorry for being so nitypicky @marinavega but can you review the test, I think it is not working as expected, or I'm being lost somewhere...
| it "doesn't create any project" do | ||
| expect do | ||
| command.call | ||
| end.not_to(change { Project.where(budget: budget).where(scope: scope).count }) |
There was a problem hiding this comment.
I'm not sure I'm understanding this test.
You've created a new, scope, but you are not using it anywere, and the let is without ! so it never gets created. The form double is initialized with scope_id: "" instead of scope, and also the name of the form attribute is decidim_scope_id, not scope_id so not sure if this is being setted correctly.
In the end the command executes all_proposals.where(decidim_scope_id: form.scope_id) and filters proposals were the scope_id equals an empty string, instead of the scope that was introduced for testing this situation but never gets created.
So I think the test succeeds accidentally but not because it executes as expected.
can you please give it a check and clarify my mind 😆
There was a problem hiding this comment.
Don't worry, this is actually super helpful! 😄 Let me check this so I can properly answer you all, guys 🙏🏻
There was a problem hiding this comment.
@Leusev @tramuntanal Ok, I didn't update all instances when renaming decidim_scope_id to scope_id.
I'll fix this 🙏🏻 TY again
Leusev
left a comment
There was a problem hiding this comment.
Good job @marinavega , just a couple of doubts..
Thanks !!
decidim-budgets/app/forms/decidim/budgets/admin/project_import_proposals_form.rb
Outdated
Show resolved
Hide resolved
decidim-budgets/app/forms/decidim/budgets/admin/project_import_proposals_form.rb
Outdated
Show resolved
Hide resolved
|
@marinavega there are some questions to be addressed in order to progress with the review, thanks cc/ @mrcasals |
Leusev
left a comment
There was a problem hiding this comment.
Thanks a lot for have been addressed either doubts as issues @marinavega 👍
From my side, everything looks correct. What about you @tramuntanal ?
|
LGTM @Leusev |
🎩 What? Why?
Adds a selector to filter accepted proposals by scope in Import proposals to project.
📌 Related Issues
📋 Subtasks
scopeselectorproposalsby scope📷 Screenshots (optional)