Details

Reviewers

ashoat
atul

Commits

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

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:

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

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Sun, Mar 12, 10:52 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptSun, Mar 12, 10:52 PM

ginsu edited the summary of this revision. (Show Details)Sun, Mar 12, 10:55 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: ashoat, atul.

ginsu added a parent revision: D7053: [lib] introduce getAvatarForUser in avatar-utils.

ginsu attached a referenced file: F424882: Screenshot 2023-03-13 at 1.27.08 AM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)

ginsu added inline comments.Sun, Mar 12, 10:58 PM

keyserver/src/fetchers/user-fetchers.js
245 ↗	(On Diff #23659)
253 ↗	(On Diff #23659)
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

ginsu attached a referenced file: F424891: Screenshot 2023-03-09 at 7.05.38 PM.png. (Show Details)Sun, Mar 12, 10:58 PM

ginsu attached a referenced file: F424888: Screenshot 2023-03-09 at 7.05.48 PM.png. (Show Details)

Harbormaster completed remote builds in B17271: Diff 23659.Sun, Mar 12, 11:08 PM

ginsu requested review of this revision.Sun, Mar 12, 11:08 PM

ginsu added a child revision: D7055: [native] introduce Avatar component.Sun, Mar 12, 11:10 PM

ashoat requested changes to this revision.Mon, Mar 13, 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.Mon, Mar 13, 9:31 AM

ginsu added inline comments.Mon, Mar 13, 3:14 PM

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

address feedback

ginsu edited the summary of this revision. (Show Details)Tue, Mar 14, 8:33 AM

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F427091: Screenshot 2023-03-14 at 11.08.57 AM.png. (Show Details)

ginsu attached a referenced file: F427092: Screenshot 2023-03-14 at 11.12.09 AM.png. (Show Details)

ginsu added inline comments.Tue, Mar 14, 8:38 AM

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

Harbormaster completed remote builds in B17316: Diff 23716.Tue, Mar 14, 8:47 AM

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

(back to your queue pending changes)

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

ginsu removed a child revision: D7055: [native] introduce Avatar component.Mon, Mar 20, 7:12 AM

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

ginsu removed a parent revision: D7053: [lib] introduce getAvatarForUser in avatar-utils.Mon, Mar 20, 7:52 AM

address comments

ginsu added inline comments.Mon, Mar 20, 2:07 PM

keyserver/src/fetchers/user-fetchers.js
253 ↗	(On Diff #23886)	We need to spread `currentUserInfo` here for flow

Harbormaster completed remote builds in B17432: Diff 23886.Mon, Mar 20, 2:19 PM

improvements

atul added inline comments.Mon, Mar 20, 2:33 PM

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

ginsu added inline comments.Mon, Mar 20, 2:43 PM

keyserver/src/fetchers/user-fetchers.js
247 ↗	(On Diff #23889)	A future diff will correctly set the min code version

Harbormaster completed remote builds in B17434: Diff 23888.Mon, Mar 20, 2:44 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.Mon, Mar 20, 2:46 PM

Harbormaster completed remote builds in B17435: Diff 23889.Mon, Mar 20, 2:57 PM

fix gates

atul accepted this revision.Mon, Mar 20, 3:10 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.