feat: specify datastore keys permission#47
Conversation
# Conflicts: # i18n/en.pot # src/components/sections/general/android-settings-container.js # src/components/sections/general/general-form.js # src/pages/layout.js
amcgee
left a comment
amcgee
left a comment
There was a problem hiding this comment.
This is a complex change, difficult to review, but I made a few suggestions after looking at the code. So long as this is well-tested for proper functionality this should be OK
| <p className={disable ? warning.warning_color : undefined}> | ||
| {disable | ||
| ? i18n.t( | ||
| "You don't have the authority to set up the android settings to this instance" |
There was a problem hiding this comment.
This should be "You are not authorized to configure Android settings, please contact a DHIS2 system administrator." (cc @cooper-joe to improve on my wording)
| "You don't have the authority to set up the android settings to this instance" | ||
| ) | ||
| : i18n.t( | ||
| 'To set up the default settings and apply to all devices, click "Set default and save"' |
| } | ||
| }) | ||
| } | ||
| return getAuthority().then(hasAuthority => { |
There was a problem hiding this comment.
It would be better not to n'est a bunch of callbacks here, this is very difficult to understand at a glance. I would recommend splitting it out into well-named helper functions
| } | ||
| }) | ||
| } | ||
| res.value.globalSettings.disableAll = !hasAuthority |
There was a problem hiding this comment.
It's better to treat this as constant, rather than mutating it - so you should create a copy below.
|
@amcgee I've just added some changes to fix it. |
|
Milagros, you can merge this but please create an issue to address the wording change I suggested, as well as the mutation of res.value in apiLoadSettings |
Thanks Austin, I've just created issues for these comments. |
2 participants
This PR has: