fixes: Change ProfileId defaulting to be an invalid profile #5730
+20
−7
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
jainv4156
changed the title
adhiamboperes
requested changes
Mar 4, 2025
Comment on lines
+136
to
+140
| reserved 1; | ||
| oneof type { | ||
| int32 internal_id = 2; | ||
| // Boolean if user is logged_out or not. | ||
| bool logged_out = 3; |
There was a problem hiding this comment.
Each field should have a comment, and implement consistent spacing with the rest of the file.
Comment on lines
41
to
+44
| fun Bundle.decorateWithUserProfileId(profileId: ProfileId) { | ||
| putProto(PROFILE_ID_BUNDLE_DECORATOR, profileId) | ||
| if (!profileId.loggedOut) { | ||
| putProto(PROFILE_ID_BUNDLE_DECORATOR, profileId) | ||
| } |
There was a problem hiding this comment.
I think it is better to enforce the check for logged out state at function callsite, rather than such a high level as here.
| if (extraControlsTitle != null) { | ||
| activity.loadProfileEdit(profileId = profileId, profileName = extraControlsTitle) | ||
| if (isProfileDeletionDialogVisible && profileId.internalId != 0) { | ||
| if (isProfileDeletionDialogVisible && !profileId.loggedOut) { |
There was a problem hiding this comment.
This is not correct, profileId.internalId != 0 here is checking if the profile is an admin, not if the profile is logged out. Remember that currently, profileId == 0 is an admin profile. -1/DefaultInstance is logged out.
| @@ -76,7 +76,8 @@ class AdministratorControlsActivity : | |||
| // TODO(#661): Change the default fragment in the right hand side to be EditAccount fragment in the case of multipane controls. | |||
| PROFILE_LIST_FRAGMENT | |||
| } | |||
| val selectedProfileId = intent?.extractCurrentUserProfileId() ?: ProfileId.getDefaultInstance() | |||
| val selectedProfileId = intent?.extractCurrentUserProfileId() ?: ProfileId | |||
| .cnewBuilder().setLoggedOut(true).build() | |||
There was a problem hiding this comment.
Suggested change
| .cnewBuilder().setLoggedOut(true).build() | |
| .newBuilder().setLoggedOut(true).build() |
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.
Explanation
fixes : 5482
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: