Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Clear unsaved groups when switching user #7236

Merged
merged 2 commits into from Mar 22, 2019
Merged

FIX: Clear unsaved groups when switching user #7236

merged 2 commits into from Mar 22, 2019

Conversation

venarius
Copy link
Contributor

This clears unsaved groups when admin switches to another user without saving.

Resolves: https://meta.discourse.org/t/admin-user-group-membership-edition-persists-between-users/112239

Didn't write a test for it since I didn't find any acceptance tests for custom groups. Just tell me when I should write some.

@discoursebot
Copy link

You've signed the CLA, venarius. Thank you! This pull request is ready for review.

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/admin-user-group-membership-edition-persists-between-users/112239/2

@eviltrout
Copy link
Contributor

Unfortunately we will rarely accept a fix without a test. If you need some clues on how to add a test @jjaffeux can probably help you out.

@ZogStriP
Copy link
Member

ZogStriP commented Mar 21, 2019

Didn't write a test for it since I didn't find any acceptance tests for custom groups. Just tell me when I should write some.

Fixing a bug or a regression is a very good reason to add/fix a test 😉

@venarius
Copy link
Contributor Author

cc @ZogStriP @eviltrout took me quite some time but I got it figured out 🙂

@@ -19,6 +19,7 @@ export default Discourse.Route.extend({
controller.setProperties({
originalPrimaryGroupId: model.get("primary_group_id"),
availableGroups: this._availableGroups,
customGroupIdsBuffer: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, are these properties the only ones we need to reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a look at it and check if there are more things that need to be reset 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZogStriP I tried a lot of things but all other user settings seem fine to me

@ZogStriP ZogStriP merged commit da187f0 into discourse:master Mar 22, 2019
@venarius venarius deleted the clear_unsaved_groups_when_switching_user branch March 22, 2019 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants