initial_snapshot: Add emailAddressVisibility override in InitialSnapshot method #1011
Conversation
| @@ -804,7 +804,7 @@ InitialSnapshot initialSnapshot({ | |||
| zulipMergeBase: zulipMergeBase ?? recentZulipVersion, | |||
| alertWords: alertWords ?? ['klaxon'], | |||
| customProfileFields: customProfileFields ?? [], | |||
| emailAddressVisibility: EmailAddressVisibility.everyone, | |||
| emailAddressVisibility: emailAddressVisibility ?? EmailAddressVisibility.everyone, | |||
There was a problem hiding this comment.
initial_snapshot: Add emailAddressVisibility override in InitialSnapshot method
The InitialSnapshot method does not override the
emailAddressVisibility in the example_data.dart
with the value passed in the function arguments.
This commit fixes the bug
Commit message nit: I think the paragraph ("The InitialSnapshot method…") isn't necessary; it just repeats what's already clear in the code change. Instead of that paragraph, how about:
- "It looks like this was left out by accident." (or something like that)
- Say whether or not any callers were passing
emailAddressVisibility. That helps make clear what changes this is making to our tests' behavior, if any. (From a quick search, I think none of the callers were passing it.)
There was a problem hiding this comment.
Okay, thanks for the comment, I will update the commit message.
Yes, at the moment no callers were passing emailAddressVisibility, but when I was working on the @mention autocomplete items, I was trying to write a test that required passing the emailAddressVisibility, that is when I discovered the issue, Even if it is not used now, I think there will be future scenarios where it would be needed.
There was a problem hiding this comment.
Makes sense. Yes, this is still a good change to make! :) Just that it's helpful to know if it's a change that affects behavior of existing tests. It sounds like it doesn't, and that's helpful to write down in the commit message, for anyone who's reading it who doesn't know that yet.
There was a problem hiding this comment.
Please post on the PR thread when you've made the change and it's ready for another review.
There was a problem hiding this comment.
Hi @chrisbobbe
I have made updates to the commit, please review.
fombalang
force-pushed
the
initial-snapshot
branch
from
dcfc369 to
a82818a
Compare
| @@ -804,7 +804,7 @@ InitialSnapshot initialSnapshot({ | |||
| zulipMergeBase: zulipMergeBase ?? recentZulipVersion, | |||
| alertWords: alertWords ?? ['klaxon'], | |||
| customProfileFields: customProfileFields ?? [], | |||
| emailAddressVisibility: EmailAddressVisibility.everyone, | |||
| emailAddressVisibility: emailAddressVisibility ?? EmailAddressVisibility.everyone, | |||
There was a problem hiding this comment.
Thanks! And one more thing I missed last time. In the commit message summary line—
initial_snapshot: Add emailAddressVisibility override in InitialSnapshot method
- Keep it to 76 characters or shorter
- Let's use "test:" for the prefix, so it's clear it's purely about test code
- The method's name is
initialSnapshot, notInitialSnapshot. Our convention is to import the example-data methods aseg., soeg.initialSnapshotis a compact way to identify the method that's nice and clear that it's an example-data method.
So, how about:
test: Fix eg.initialSnapshot to use emailAddressVisibility override
There was a problem hiding this comment.
Okay, thank you. Will update that.
There was a problem hiding this comment.
I have updated the commit message
fombalang
force-pushed
the
initial-snapshot
branch
from
a82818a to
f8b7119
Compare
|
Thanks, LGTM! Marking for Greg's review. |
chrisbobbe
added
the
integration review
|
Thanks @fombalang for the fix, and @chrisbobbe for the previous reviews! Looks good; merging. |
I think we forgot this. No callers in the tests currently use this override, but it's probably a good idea to have it properly overriding anyway.
gnprice
force-pushed
the
initial-snapshot
branch
from
f8b7119 to
ffa41be
Compare
I noticed a bug where the value passed for the
emailAddressVisibilityin theinitialSnapshotmethod in theexample_data.dartfile was not being passed down to the class but rather defaulted toEmailAddressVisibility.everyoneeven though a different value was passed. When I looked deeper into it, I noticed that they forgot to add the override. This PR adds a fix for that. There isn't an open issue for this yet, so I am unable to link to it.Please review when available.
Thank you.