Change plugins enabling in report flow to checkboxes #3747
Conversation
|
|
||
| for selected_plugin in selected_plugins: | ||
| try: | ||
| get_katalogus(self.organization.code).enable_boefje_by_id(selected_plugin) |
There was a problem hiding this comment.
See earlier remark about the client
| @@ -18,7 +18,7 @@ | |||
| {% endfor %} | |||
| {% endif %} | |||
| {% for required_optional_plugin, plugins_ in plugins.items %} | |||
| {% for plugin in plugins_ %}<input type="hidden" name="plugin" value="{{ plugin.id }}">{% endfor %} | |||
| {% for plugin in plugins_ %}<input type="hidden" name="all_plugins" value="{{ plugin.id }}">{% endfor %} | |||
There was a problem hiding this comment.
Just to be sure: The views still seem to refer to the plugin query parameter, so I'm unsure if this change works. Regardless, I think plugins as a query parameter list would've been a better name
There was a problem hiding this comment.
Yeah weird right. It seems that this variable is and was not used... But I'm afraid to delete it.
Checklist for QA:
What works:Seems to work. Couldn't find anything that breaks obviously. What doesn't work:n/a Bug or feature?:n/a |
| for selected_plugin in selected_plugins: | ||
| try: | ||
| client.enable_boefje_by_id(selected_plugin) | ||
| except HTTPError: |
There was a problem hiding this comment.
I hate that we bubble up the http errors from the client to here. Lets move that away asap, the katalogusclient already has some localized exceptions for other issues.
| def post(self, request, *args, **kwargs): | ||
| selected_plugins = request.POST.getlist("plugin", []) | ||
|
|
||
| client = get_katalogus(self.organization.code) | ||
| for selected_plugin in selected_plugins: | ||
| try: | ||
| client.enable_boefje_by_id(selected_plugin) | ||
| except HTTPError: | ||
| messages.error( | ||
| request, | ||
| _("An error occurred while enabling {}. The plugin is not available.").format(selected_plugin), | ||
| ) | ||
| return self.post(request, *args, **kwargs) | ||
| return super().post(request, *args, **kwargs) | ||
|
|
There was a problem hiding this comment.
Can you move this post to the ReportFinalSettingsView in base.py? Then it will be implemented for aggregate and multireport
There was a problem hiding this comment.
We need to make sure that the user knows that by checking the checkboxes that it means enable and continue
There was a problem hiding this comment.
Hmm no I cant move it to there because these plugins are enabled only in the export step, and not taken all the way to the final step like the other settings.
And the button already says: enable and continue
|
Permission checking now works. Can be tested by:
|
|
noamblitz
changed the title
|
I did a QA test and this was the result of it: WHAT WORKS
What doesn't work:
|
Changes
This PR changes the plugin tiles in the report flow to use checkboxes. This fixes the report flow because the checkboxes use a post request.
Issue link
Please add the link to the issue after "Closes".
Closes ...
Demo
QA notes
There might be regressions in the report flow because I renamed a variable that I think is unused, but I am not sure about that.
Code Checklist
.envchanges files if required and changed the.env-distaccordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.