Page MenuHomePhabricator

[keyserver/lib] fetch avatar column from users table in fetchCurrentUserInfo
ClosedPublic

Authored by ginsu on Mar 12 2023, 10:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 18, 5:37 AM
Unknown Object (File)
Sat, Jun 15, 12:59 AM
Unknown Object (File)
Fri, Jun 14, 11:12 PM
Unknown Object (File)
Fri, Jun 14, 11:12 AM
Unknown Object (File)
Fri, Jun 14, 6:11 AM
Unknown Object (File)
Fri, Jun 14, 4:56 AM
Unknown Object (File)
Fri, Jun 14, 4:46 AM
Unknown Object (File)
Wed, Jun 12, 8:35 PM
Subscribers

Details

Summary

The current user will need the avatar info from the db. Updated the query to include avatar in the returned object whenever avatar is not null. A subsequent diff will address the other queries.

This diff won't be landed until there is a feature flag for the avatars work


Linear Task: https://linear.app/comm/issue/ENG-3209/update-user-fetchers-to-return-avatar-info-from-users-table

Test Plan

I logged out currentUserInfo in profile-screen.react

When avatar is null:

Screenshot 2023-03-14 at 11.08.57 AM.png (130×694 px, 67 KB)

When avatar is populated (hardcoded an avatar variable on the keyserver just for the logs)

Screenshot 2023-03-14 at 11.12.09 AM.png (136×1 px, 104 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, atul.
ginsu edited the summary of this revision. (Show Details)
keyserver/src/fetchers/user-fetchers.js
245 ↗(On Diff #23659)

Screenshot 2023-03-09 at 7.05.38 PM.png (1×3 px, 2 MB)

253 ↗(On Diff #23659)

Screenshot 2023-03-09 at 7.05.48 PM.png (1×3 px, 2 MB)

lib/types/user-types.js
52 ↗(On Diff #23659)

Again we will update this type in a subsequent diff when the logic for avatars with images are introduced

ashoat requested changes to this revision.Mar 13 2023, 9:31 AM
ashoat added inline comments.
keyserver/src/fetchers/user-fetchers.js
231 ↗(On Diff #23659)

See below – we should not include avatar in UserInfo if it is not set

lib/types/user-types.js
52 ↗(On Diff #23659)

This needs to be optional. Otherwise you are going to cause currentUserInfo inconsistencies for literally every active client, since they will all be missing avatar: undefined that keyserver expects them to have. The state check mechanism will identify these inconsistencies and force every client to reload its currentUserInfo

This revision now requires changes to proceed.Mar 13 2023, 9:31 AM
lib/types/user-types.js
52 ↗(On Diff #23659)

Got it. Thinking about this again with a better understanding I see what you mean. This comment on linear from earlier today really helped me better understand how/when we need to think about older clients

keyserver/src/fetchers/user-fetchers.js
248 ↗(On Diff #23716)

Personally, I am a pretty big fan of this syntax to conditionally add fields to an object and think it is pretty readable, but I am aware that some in the dev community may disagree. If we don't like this syntax I can change it

https://stackoverflow.com/questions/11704267/in-javascript-how-to-conditionally-add-a-member-to-an-object

ashoat added 1 blocking reviewer(s): atul.

Eh, I don't love it but I won't veto. @atul what do you think?

Ehhh I'd really prefer if we stayed away from these syntax tricks. I think spreading and destructuring can already get a little tricky on occasion (eg nested restructuring, reassigning names, etc) and I can see this just further confusing things.. especially for new devs

I'd really prefer for things to be as explicit as possible (even if it's a few more lines of code) + we already have patterns for doing this elsewhere in the codebase so it'd be best to be consistent

(Sorry for extra churn + being a buzzkill lol)

Okay sounds good i'll have an update out shortly with a cleaner syntax

atul requested changes to this revision.Mar 14 2023, 1:47 PM

(back to your queue pending changes)

This revision now requires changes to proceed.Mar 14 2023, 1:47 PM

Sequencing this diff towards the end of the diff stack (need to put up a few more diffs up first) but will update the relational revisions when I have a better idea

keyserver/src/fetchers/user-fetchers.js
253 ↗(On Diff #23886)

We need to spread currentUserInfo here for flow

keyserver/src/fetchers/user-fetchers.js
249–251 ↗(On Diff #23888)

Don't we need to gate this on client version?

added avatar client feature gate to fetcher

keyserver/src/fetchers/user-fetchers.js
247 ↗(On Diff #23889)

A future diff will correctly set the min code version

atul requested changes to this revision.Mar 20 2023, 2:46 PM

One last change

keyserver/src/fetchers/user-fetchers.js
247 ↗(On Diff #23889)

One last change...

for featureGateSettings it's !hasMinCodeVersion but for featureGateAvatars it's hasMinCodeVersion. Can we make this consistent?

This revision now requires changes to proceed.Mar 20 2023, 2:46 PM
atul added inline comments.
keyserver/src/fetchers/user-fetchers.js
247–251 ↗(On Diff #23890)

Would prefer something like the above to prevent the whole featureGate* pattern and double negation.. but this logic is correct as is.

This revision is now accepted and ready to land.Mar 20 2023, 3:10 PM
keyserver/src/fetchers/user-fetchers.js
247–251 ↗(On Diff #23890)

Okay yea I agree, switching up the language makes it a lot better

implemented atuls quick suggestion