XWIKI-22010: The rights buttons for Unregistered Users do not display anymore the value set after page refresh #3006
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
… anymore the value set after page refresh * Renamed the r variable to a more explicit name : guestRights * Replaced the string value when calling MSCheckbox for guest users with an int value, like it is done for any other user * Indented blocks with a wrong indentation
manuelleduc
approved these changes
Mar 21, 2024
There was a problem hiding this comment.
lgtm. @Sereza7 do you mind splitting this is two commits?
- one for the actual fix
- one
[Misc]for the code style changes
Thanks!
… display anymore the value set after page refresh" This reverts commit 9c48aeb.
… anymore the value set after page refresh * Replaced the string value when calling MSCheckbox for guest users with an int value, like it is done for any other user
|
I reverted the previous commit and added back only the necessary change here 👍 Started a PR with the misc changes at #3009 :) |
It's also acceptable to rework the history locally and "force-with-lease", I would I use "rebase and merge", but that works too. Thanks a lot! |
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.
Jira URL
https://jira.xwiki.org/browse/XWIKI-22010
Changes
Description
Clarifications
This regression is caused because the MSCheckbox instantiation is done with a state of "1" (string) instead of 1 (int) for unregistered users.
This is inconsistent with the regular user/group MSCheckbox instantiation that used a numeric value. Beforehand it passed successfully because we used it as a list index. Now the check on the value doesn't pass and it's initialized with the default state : none.
I could think of 2 solutions:
Fix the state selector to accept int and string as a state.
Fix the MSCheckbox call to provide only ints for the state.
could be considered as leaving a breaking change in. However the MSCheckbox class is documented nowhere. Because of that I suppose it's not part of our public API, so as long as we fix all internal uses, it's okay to update it.
I'd rather keep variable's type consistent here, so I'll go with 2.
Screenshots & Video
22010-demo.mp4
We can see on this video that unlike before, the state of the unregistered user checkboxes are correctly initialized after a refresh.
Executed Tests
Only manual tests, see demo above. No automatic tests were broken by this regression so I suppose fixing it does not break anything. Most of the changes in this PR are codestyle only.
Expected merging strategy