fix(systemtags): unify restrict_creation_to_admin handling #51288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: skjnldsv <[email protected]>
skjnldsv
requested review from
artonge,
nfebe and
sorbaugh
and removed request for
a team
skjnldsv
force-pushed
the
fix/admin-tag-color-prevent
branch
from
6ef85aa to
9f77664
Compare
Signed-off-by: skjnldsv <[email protected]>
skjnldsv
force-pushed
the
fix/admin-tag-color-prevent
branch
from
9f77664 to
e41d716
Compare
Signed-off-by: skjnldsv <[email protected]>
Signed-off-by: skjnldsv <[email protected]>
susnux
reviewed
Mar 5, 2025
| import { elementColor, invertTextColor, isDarkModeEnabled } from '../utils/colorUtils' | ||
| import { getNodeSystemTags, setNodeSystemTags } from '../utils' |
There was a problem hiding this comment.
Suggested change
| import { getNodeSystemTags, setNodeSystemTags } from '../utils' | |
| import { getNodeSystemTags, setNodeSystemTags } from '../utils.ts' |
susnux
reviewed
Mar 5, 2025
| @@ -204,6 +209,8 @@ export default defineComponent({ | |||
| emit, | |||
| Status, | |||
| t, | |||
| // Either tag creation is not restricted to admins or the current user is an admin | |||
| canEditOrCreateTag: !restrictSystemTagsCreationToAdmin || getCurrentUser()?.isAdmin, | |||
susnux
reviewed
Mar 5, 2025
| @@ -189,7 +189,8 @@ export default Vue.extend({ | |||
| this.sortedTags.unshift(createdTag) | |||
| this.selectedTags.push(createdTag) | |||
| } catch (error) { | |||
| if(loadState('settings', 'restrictSystemTagsCreationToAdmin', '0') === '1') { | |||
| const systemTagsCreationRestrictedToAdmin = loadState<true|false>('settings', 'restrictSystemTagsCreationToAdmin', false) === true | |||
There was a problem hiding this comment.
Simple types here (should be boolean), but those simple types are also inferred from default value (false) so it correctly infers boolean.
Suggested change
| const systemTagsCreationRestrictedToAdmin = loadState<true|false>('settings', 'restrictSystemTagsCreationToAdmin', false) === true | |
| const systemTagsCreationRestrictedToAdmin = loadState('settings', 'restrictSystemTagsCreationToAdmin', false) |
susnux
reviewed
Mar 5, 2025
| </p> | ||
|
|
||
| <NcCheckboxRadioSwitch type="switch" | ||
| :checked.sync="systemTagsCreationRestrictedToAdmin" | ||
| @update:checked="updateSystemTagsDefault"> | ||
| {{ t('settings', 'Restrict tag creation to admins only') }} | ||
| {{ t('settings', 'Restrict tag edition and creation to admins only') }} |
There was a problem hiding this comment.
I think it is
Suggested change
| {{ t('settings', 'Restrict tag edition and creation to admins only') }} | |
| {{ t('settings', 'Restrict tag editing and creation to admins only') }} |
There was a problem hiding this comment.
I checked and you're right! What a weird phrazing 😁
I would even go for
Suggested change
| {{ t('settings', 'Restrict tag edition and creation to admins only') }} | |
| {{ t('settings', 'Restrict tag creation and editing to administrators only') }} |
susnux
reviewed
Mar 5, 2025
| data() { | ||
| return { | ||
| // By default, system tags creation is not restricted to admins | ||
| systemTagsCreationRestrictedToAdmin: loadState('settings', 'restrictSystemTagsCreationToAdmin', '0') === '1', | ||
| systemTagsCreationRestrictedToAdmin: loadState<true|false>('settings', 'restrictSystemTagsCreationToAdmin', false) === true, |
There was a problem hiding this comment.
Suggested change
| systemTagsCreationRestrictedToAdmin: loadState<true|false>('settings', 'restrictSystemTagsCreationToAdmin', false) === true, | |
| systemTagsCreationRestrictedToAdmin: loadState('settings', 'restrictSystemTagsCreationToAdmin', false), |
susnux
reviewed
Mar 5, 2025
| @@ -206,6 +208,11 @@ public function updateTag( | |||
| ); | |||
| } | |||
|
|
|||
| $user = $this->userSession->getUser(); | |||
| if (!$this->canUserCreateTag($user)) { | |||
| throw new TagUpdateForbiddenException('Tag update forbidden'); | |||
There was a problem hiding this comment.
IMHO the message is basically the exception class name, so could also drop it.
Suggested change
| throw new TagUpdateForbiddenException('Tag update forbidden'); | |
| throw new TagUpdateForbiddenException(); |
susnux
reviewed
Mar 5, 2025
| * @param IUser|null $user user to check permission for | ||
| * @return bool true if the user is allowed to update a tag, false otherwise | ||
| * | ||
| * @since 31.0.0 |
There was a problem hiding this comment.
Suggested change
| * @since 31.0.0 | |
| * @since 31.0.1 |
susnux
reviewed
Mar 5, 2025
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors |
There was a problem hiding this comment.
Suggested change
| * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors | |
| * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors |
susnux
reviewed
Mar 5, 2025
| /** | ||
| * Exception when a user doesn't have the right to create a tag | ||
| * | ||
| * @since 31.0.0 |
There was a problem hiding this comment.
Suggested change
| * @since 31.0.0 | |
| * @since 31.0.1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #51233
Fix #51247
false) and value handling asbool(previouslystring)