-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add form-assign-proposal-to-valuator to proposal page #24
base: release/0.26-stable
Are you sure you want to change the base?
add form-assign-proposal-to-valuator to proposal page #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @antopalidi
This works perfectly, a couple of improvements before closing:
- Currently, when you assign a valuator you return to the proposal's list page. We should stay in the same page. The method receiving the assignation should distinct the referer page or, we should add a new action to the controller to handle this assignation and stay in the page. Whatever option is easier.
- We need to add a test for this behaviour to
admin_manages_proposal_valuators_spec.rb
oradmin_manages_proposals_spec.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @antopalidi
please check the comments for the things that need a fix.
Additionally, I think we should move the "assign new valuator" to the bottom of the list as it was at the beginning.
Thanks!
decidim-admin/app/packs/stylesheets/decidim/admin/modules/_forms.scss
Outdated
Show resolved
Hide resolved
decidim-proposals/app/controllers/decidim/proposals/admin/valuation_assignments_controller.rb
Outdated
Show resolved
Hide resolved
decidim-proposals/app/controllers/decidim/proposals/admin/valuation_assignments_controller.rb
Outdated
Show resolved
Hide resolved
decidim-proposals/spec/system/admin/valuator_manages_proposals_spec.rb
Outdated
Show resolved
Hide resolved
decidim-proposals/spec/system/admin/valuator_manages_proposals_spec.rb
Outdated
Show resolved
Hide resolved
decidim-proposals/app/views/decidim/proposals/admin/proposals/show.html.erb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @antopalidi
very good, there's only 2 details I left on the comments.
After that we're good!!
decidim-proposals/app/controllers/decidim/proposals/admin/valuation_assignments_controller.rb
Outdated
Show resolved
Hide resolved
decidim-proposals/spec/system/admin/admin_manages_proposal_valuators_spec.rb
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excelent!
Note that we won't merge this branch, instead we've created a parallel PR #26 to the branch 0.26-lucerne
. That's the one that will be used for their application.
This PR will remain in "Draft" status until we need it again in order to port it to the 0.27 version and the develop
branch in the future (when new features are accepted again in Decidim)
fixes #20