Conversation
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Nicely done. I like this a lot better.
There are a couple of minor issues, like a missing translation.
I also have some other small nitpicks and some suggested potential refactors, but we don't necessarily need to tackle those now, since it is no worse than it was before.
|
|
||
| if ( | ||
| setting['name'] == 'newsletter-settings' | ||
| and not request.app.org.show_newsletter | ||
| ): | ||
| continue | ||
|
|
||
| if ( | ||
| setting['name'] == 'ris-settings' | ||
| and not request.app.org.ris_enabled | False | ||
| ): | ||
| continue |
There was a problem hiding this comment.
It might not be quite worth it yet, but if we see more conditions sneaking in like this, we may want to extract this into an optional setting_visible predicate that takes a callback (lambda request: True by default) that needs to return True in order for the setting to be visible.
But if you feel like cleaning this up a bit, please go ahead.
Furthermore in terms of refactors, that might be worth it: Defining a new directive for settings views, with its own registry (or rather making the one we didn't end up using work correctly). So we don't have to use quite as much work to retrieve all of the setting views. This could be a CompositeAction that yields a HtmlHandleFormAction and a new action that registers the setting or a subclass of HtmlHandleFormAction that adds the extra logic for registering the setting.
We could then also be a little bit more robust and emit an exception if we encounter a setting with a setting category we don't know about.
| @OrgApp.form( | ||
| model=Organisation, name='module-activation-settings', template='form.pt', | ||
| permission=Secret, form=ModuleActivationSettingsForm, | ||
| setting=_('Activate/deactivate modules'), order=0, |
There was a problem hiding this comment.
I found this to be a little wordy and it also ends up not fitting into one line in the settings view and gets broken up weirdly, on the other hand it's very clear what it means and I can't think of something shorter that still explains what the setting is for. Maybe "Optional Modules" could work?
There was a problem hiding this comment.
I like "optional Modules". I will leave the "activate/deactivate Modules" in the modules dropdown and rename it in the settings.
2 participants
Please fill in the commit message below and work through the checklist. You can delete parts that are not needed, e.g. the optional description, the link to a ticket or irrelevant options of the checklist.
Commit message
Org: Rearrange settings and management bar
TYPE: Feature
Checklist