-
Notifications
You must be signed in to change notification settings - Fork 371
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
Add public get tags method #1893
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I believe the code is correct, but for future proofing lets add a test where the user changes (causing the tags to change) and verify that when we switch users we get the updated tags.
@emawby Good point! A new test case for tags modification is added in this PR. Please have a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the linter is failing. You may need to rebase since changes were merged that fix linting
Got rid of userManager.tags getter because it conflicts the name generated by the JVM which is also getTags
5250d84
to
4702341
Compare
I noticed that the linter check is still failing, could you fix the linter indentation errors in UserManagerTests? Also just a nit but, I think the Migration Guide could be updated to include |
This reverts commit 2cdc61f.
@shepherd-l Thanks, Shepherd! I have fixed the linter error and added 'getTags' in the Migration Guide. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Reviewed 1 of 3 files at r2.
Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @jennantilla, @jkasten2, and @nan-li)
Description
One Line Summary
Add a public getTags method
Details
Motivation
Provide a public getTags upon request
Scope
We are only returning the local tags
Testing
Unit testing
Add one unit test to ensure the data is the same
Manual testing
I ran it in an emulator, added a tag, and the call of getTags returns that tag
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is