Drop three User fields we aren't keeping up to date; verify that's all #876
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
PIG208
reviewed
Aug 10, 2024
lib/model/store.dart
Outdated
| @@ -465,6 +465,10 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore { | |||
| if (user == null) { | |||
| return; // TODO log | |||
| } | |||
| // Fields on [User] not updated here: | |||
| // userId, dateJoined, isBot, botType, isSystemBot | |||
There was a problem hiding this comment.
Do we want to have a reference on User, so that we remember to update this comment when there is another immutable field?
There was a problem hiding this comment.
Well, the more important case is when adding a mutable field, so that we add the code to update it.
Anyway, switched to a comment on User, in conjunction with making the immutable fields final.
sm-sayedi
reviewed
Aug 11, 2024
lib/model/store.dart
Outdated
Comment on lines
468
to
469
| // Fields on [User] not updated here: | ||
| // userId, dateJoined, isBot, botType, isSystemBot |
There was a problem hiding this comment.
What about also making these immutable fields final?!
There was a problem hiding this comment.
Good idea, thanks. Switched to that.
gnprice
force-pushed
the
pr-user-update
branch
from
b30bfa7 to
429fa9e
Compare
These are obsoleted by [User.role] (which dates to Zulip 4.0). We never looked at these fields, but had a latent bug because we weren't keeping them up to date on RealmUserUpdateEvent. To fix the bug, just stop having the fields around at all. (Later if we find we want booleans with these handy names, we can always add getters that just inspect [User.role].)
This was prompted by noticing recently that we weren't updating [User.isActive]: zulip#816 (comment) After fixing that, I looked and found there were actually three other fields on User which we weren't updating -- a latent bug, since we never looked at those fields, but a bug. Fixed that in the preceding commit. Now the remaining fields that don't get updated are fields that really are immutable on a Zulip user. Mark those as final, and add a comment to help maintain the pattern.
chrisbobbe
force-pushed
the
pr-user-update
branch
from
429fa9e to
156c693
Compare
|
Thanks, LGTM! Merged. |
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.
This was prompted by noticing recently that we weren't updating
[User.isActive]:
#816 (comment)
(/cc @sm-sayedi)
After we fixed that, I looked and found there were actually three
other fields on User which we weren't updating -- a latent bug,
since we never looked at those fields, but a bug. Fix that.
Then add a comment showing my work on verifying that that's it,
and giving us a head start on re-verifying that in the future
with whatever's changed in the API between now and then.