-
-
Notifications
You must be signed in to change notification settings - Fork 655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show custom profile fields in users' profiles #5398
Conversation
8dbacf0
to
21c7b3a
Compare
That looks great. Much needed feature! |
Thanks for the feedback!
That is a very reasonable set of feature requests. The way Zulip's custom profile fields work, each field has a type (like "some text", "a date", "a list of other Zulip users"): https://zulip.com/help/add-custom-profile-fields#profile-field-types That's helpful for giving it appropriate UI on all platforms, so I think the way we'd want to implement those is as additional types there. If you'd like to either file an issue on https://github.com/zulip/zulip , or start a thread in #feedback on https://chat.zulip.org/ , those would be good places to have that discussion. Specifically for email addresses, though, every Zulip user already has an email address associated with their account, and depending on the organization those emails may be visible to other users. We don't currently show those on this screen in the mobile app, but it seems like we should. I'll file a quick issue so we don't forget about it. (edit: -> #5400) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yay! See some small comments below.
src/api/modelTypes.js
Outdated
// A cross-realm bot has no custom profile fields set. | ||
// But there doesn't seem to be a need to enforce that in the type. | ||
+profile_data?: $ElementType<User, 'profile_data'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api types: Let CrossRealmBot have profile_data, like User
Even if in fact a cross-realm bot never has a `profile_data` property,
this type with `profile_data?: …` is still accurate. No need to make
things more complicated by encoding the distinction.
I don't object, and I understand that this still accurately describes what we get from servers, even if it's less precise. But I think 6dc16dd, which encoded the distinction, didn't go too far toward overcomplicating things.
I think there were two goals in that commit:
- Find out what the API is; don't just write what we see servers doing
- Match our types more precisely to the API
These are good goals that can help us make code less complicated and error-prone. A few examples where things got complicated are
- "Notes from studying the server code" on
PmMessage
- "The
flags
story is a bit complicated" onMessageBase
.
This change is helpful because it lets us treat User
s and CrossRealmBot
s more uniformly, in a UI that doesn't "switch/case" on whether it's rendering a user or a bot. (It shouldn't do that, should it?) And it's pretty harmless because we never construct CrossRealmBot
s ourselves, so we won't actually end up with one that has a profile_data
property, which (if we did) would be surprising and would need explaining. I think those are good reasons to relax this type in this small way.
A few nits, though:
- I think this should replace the
profile_data is never present
line, earlier in the list of properties. - I'm not sure it makes a difference, but the doc says bots can't have custom profile fields; not that they don't.
- Without more context, is it clear that "A cross-realm bot has no custom profile fields set" means that the
profile_data
property is absent? (As opposed to present with an empty object, or withnull
, etc.) I think saying this in the comment could help the reader understand what "enforce that in the type" means. (I guess the doc does say it though; maybe that's enough.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there were two goals in that commit:
- Find out what the API is; don't just write what we see servers doing
- Match our types more precisely to the API
These are good goals that can help us make code less complicated and error-prone.
Yep! I agree.
This change is helpful because it lets us treat
User
s andCrossRealmBot
s more uniformly, in a UI that doesn't "switch/case" on whether it's rendering a user or a bot.
Yeah. In general I think having the separate type CrossRealmBot
is a legacy wart -- the distinction between human users and normal per-realm bots on the one hand (typed as User
), and cross-realm bots on the other hand, is pretty much an implementation detail as far as we're concerned, and shouldn't have so much prominence in our code.
Before your recent ab4d746 and 9325bd2, there were a lot of small differences between the types, and so unifying them was a cleanup task I'd never quite finished. (In 2018 into 2019 there was a lot of confusion about this in our code, which starting with 8ef11eb I cleaned up a lot of but not all.) But now they're very similar! So it might be a reasonably short push from here to (a) use UserOrBot
everywhere, and then (b) make that be just a single object type that covers both, and call it User
.
And it's pretty harmless because we never construct
CrossRealmBot
s ourselves,
Right. These types purely describe data we get from the server -- they don't describe expectations the server has for any data it gets from us. So we can freely make the types less specific, and that's always safe. (At worst it could mean a test case supplies invalid data and we don't notice, making the test less helpful.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think this should replace the
profile_data is never present
line, earlier in the list of properties.
Thanks, yeah, I'd missed that line.
- I'm not sure it makes a difference, but the doc says bots can't have custom profile fields; not that they don't.
Yeah, in this context I think those are different ways of saying the same thing.
- Without more context, is it clear that "A cross-realm bot has no custom profile fields set" means that the
profile_data
property is absent? (As opposed to present with an empty object, or withnull
, etc.) …
No, I think it leaves it ambiguous between absent and empty-object. (The type doesn't admit null
.)
The API docs leave the same question not quite clear when it comes to realm_users
and realm_non_active_users
too. They say profile_data
is an object and not optional, so taken literally that should mean it's always present (and would just be an empty object when no fields set)… but then they say it's absent for bots, so one can't take it literally. Further, they tie its absence for bots to their not having custom profile fields, which seems to actively suggest that it might be absent for other users that have no fields too.
… I think saying this in the comment could help the reader understand what "enforce that in the type" means. (I guess the doc does say it though; maybe that's enough.)
Yeah. Quite possibly I should just delete the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, makes sense, thanks for those explanations.
| { +displayType: 'users', +userIds: $ReadOnlyArray<UserId> }; | ||
|
||
function interpretCustomProfileField( | ||
realmDefaultExternalAccounts: $ElementType<RealmState, 'defaultExternalAccounts'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
Could be $PropertyType<RealmState, 'defaultExternalAccounts'>
, I think, since you're passing a string-literal type.
…
or even RealmState['defaultExternalAccounts']
!
Glad to be on newer Flow, since the RN v0.66 upgrade. Looks like $ElementType
and $PropertyType
will even be removed in a future Flow: https://flow.org/en/docs/types/utilities/#toc-propertytype
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be
$PropertyType<RealmState, 'defaultExternalAccounts'>
, I think, since you're passing a string-literal type.
It could, but AFAICT $PropertyType
didn't have any advantages over $ElementType
, so it was better to just use a single name.
or even
RealmState['defaultExternalAccounts']
!Glad to be on newer Flow, since the RN v0.66 upgrade.
Oh, neat! Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but it looks like our Prettier version doesn't support that syntax yet:
Running prettier...
prettier-eslint [ERROR]: prettier formatting failed due to a prettier error
prettier-eslint-cli [ERROR]: There was an error formatting "/home/greg/z/mobile/src/api/modelTypes.js":
SyntaxError: Unexpected token, expected "]" (312:24)
310 | // (We could be more specific here; but in the interest of reducing
311 | // differences between CrossRealmBot and User, just follow the latter.)
> 312 | +profile_data?: User['profile_data'],
Probably #5393 will fix that. I'll switch back for this PR.
function interpretCustomProfileField( | ||
realmDefaultExternalAccounts: $ElementType<RealmState, 'defaultExternalAccounts'>, | ||
realmField: CustomProfileField, | ||
profileData: $ElementType<UserOrBot, 'profile_data'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same comment about $ElementType
)
src/users/userSelectors.js
Outdated
// TODO(server): This is completely undocumented. The key to | ||
// reverse-engineering it was: | ||
// https://github.com/zulip/zulip/blob/18230fcd9/static/js/settings_account.js#L247 | ||
const userIds: UserId[] = JSON.parse(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could use $ReadOnlyArray
export function getCustomProfileFieldsForUser( | ||
realm: RealmState, | ||
user: UserOrBot, | ||
): Array<{| +fieldId: number, +name: string, +value: CustomProfileFieldValue |}> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use $ReadOnlyArray
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the function's return type, so it's purely in an output/covariant position -- which means that for the caller Array
means more flexibility without losing any flexibility.
Sometimes that flexibility comes in handy, e.g. for wanting to sort the array. In recipient.js
in particular there've been some places where one function was returning a $ReadOnlyArray
and the caller wanted to sort it, and it required some refactoring.
Here this function is constructing the array fresh (rather than pulling it verbatim from some existing data structure), so there's no constraint requiring it to be read-only -- so we might as well leave the caller the flexibility to use that fact.
return ( | ||
<View style={styles.row}> | ||
<UserAvatar size={24} avatarUrl={user.avatar_url} /> | ||
<ZulipText style={styles.text} text={user.full_name} /> | ||
</View> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an iOS screenshot:
I noticed a few things about the user item in the "users" field type:
- Could we vertically align the center of the avatar with the center of the user's name? The text looks higher than it should be.
- Should it be a link to the user's profile?
- Should we use a placeholder image and text if you've muted the user, like we do elsewhere?
- (Show email? Probably not; takes too much space to be worth it.)
- Should we show the user's status emoji, if set?
Ah and one more:
- This is obviously a contrived example where the name is very long and without spaces. Should we handle long names by truncating or wrapping (and if wrapping, where should we wrap)?
I'm wary of making the wrong abstraction, but I wonder if this is a case where something like UserItem
should take care of handling all these subtle things, and grow a few more controls to handle callers' needs (probably one that lets us make the avatar smaller than 48, for a start). Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a few things about the user item in the "users" field type:
Yeah, I agree with all of those. Good catch re: muted user.
I wonder if this is a case where something like
UserItem
should take care of handling all these subtle things, and grow a few more controls to handle callers' needs (probably one that lets us make the avatar smaller than 48, for a start).
Yeah, I was thinking the same thing midway through reading your comment. 🙂 Will take a look.
Even if in fact a cross-realm bot never has a `profile_data` property, this type with `profile_data?: …` is still accurate. (These types purely describe data we get from the server, not any expectations the server has for any data it gets from us, so it's always safe to make them less specific.) And in general the CrossRealmBot vs. User distinction is more a legacy wart than something that's doing us any good. We've recently made progress on reconciling them, so keep on in that direction. Discussion: zulip#5398 (comment)
21c7b3a
to
a18e77a
Compare
Pushed a revision addressing everything but the user display. Looking at that next. |
Even if in fact a cross-realm bot never has a `profile_data` property, this type with `profile_data?: …` is still accurate. (These types purely describe data we get from the server, not any expectations the server has for any data it gets from us, so it's always safe to make them less specific.) And in general the CrossRealmBot vs. User distinction is more a legacy wart than something that's doing us any good. We've recently made progress on reconciling them, so keep on in that direction. Discussion: zulip#5398 (comment)
a18e77a
to
43f20b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! And I see UserItem
also gets us the presence-status indicator too, which is nice.
It looks like there are two touch targets in UserItem
, which I can see highlighted when I press them: one on the whole area covered by the UserItem
(first screenshot), one on just the avatar (second screenshot). I think we only need the former, what do you think?
Also, it looks like we're missing some padding that would give some whitespace between the left edge of the avatar and the left edge of the area covered by the item (see first screenshot).
src/api/modelTypes.js
Outdated
// (We could be more specific here; but in the interest of reducing | ||
// differences between CrossRealmBot and User, just follow the latter.) | ||
+profile_data?: $ElementType<User, 'profile_data'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the property ordering doesn't follow the doc; see
export type CrossRealmBot = {|
// Property ordering follows the doc.
// Current to feature level (FL) 121.
43f20b8
to
923980c
Compare
OK, revision pushed! |
Even if in fact a cross-realm bot never has a `profile_data` property, this type with `profile_data?: …` is still accurate. (These types purely describe data we get from the server, not any expectations the server has for any data it gets from us, so it's always safe to make them less specific.) And in general the CrossRealmBot vs. User distinction is more a legacy wart than something that's doing us any good. We've recently made progress on reconciling them, so keep on in that direction. Discussion: zulip#5398 (comment)
There's no event that updates this (because it can't actually change without a server upgrade, and consequently a server restart.) So that makes it extra straightforward to maintain.
There are two different screens where we show a variety of profile data: * When you navigate to a user's profile, we show AccountDetailsScreen. * When you go to the rightmost tab in the app's main screen, the one with your avatar as the icon, we show ProfileScreen. The names of these components aren't real helpful. But we naturally want different combinations of information on them -- in particular ProfileScreen is where one finds several pieces of UI that some users will want to use regularly, so it's important that it not get too cluttered. The two screens use a shared component AccountDetails for some of the information they both want to display. Then for some pieces of information that we want to show on AccountDetailsScreen but not on ProfileScreen, the way we'd implemented that is by having AccountDetails go ahead and show them -- but only if the user we're looking at is not the self-user. That has the side effect that if you take the trouble to go navigate to your own profile, say by visiting the self-1:1 thread and hitting the "info" icon, we still won't show you your full profile in the way it shows up for other people. That's unhelpful. Instead, have AccountDetails show the same information for any user. For things we want to leave out of ProfileScreen, leave them out of AccountDetails, and have AccountDetailsScreen handle them. This commit should make no change on ProfileScreen, and no change on AccountDetailsScreen when viewing any profile but your own. The styles added to the wrapper views mimic what the ComponentList helper (used by AccountDetails) was doing with them.
…ctors These are pretty different in flavor from the other selectors here, and because of their comments they're as long as the rest of the file together.
This brings them close to the definition of the UI whose layout they govern, and it makes it easy to start having some of the styles depend on the props.
This will let us rename the latter as simply `styles`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM! See one question below, then please merge at will. I've rebased and resolved some conflicts, so I'll push the result of that. You may want to check that I didn't mess anything up when doing so.
const user = useSelector(state => tryGetUserForId(state, userId)); | ||
|
||
const onPress = React.useCallback((user: UserOrBot) => { | ||
NavigationService.dispatch(navigateToAccountDetails(user.user_id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where reasonable, it'd be great to avoid bringing NavigationService
into more files, for #4417. Unfortunately, both before and after #5397, I've had trouble with Flow not accepting a useNavigation()
with navigation.push('account-details', { userId })
, even though I think it should be fine. Do you see an easy way to do that?
After #5397, a useNavigation()
with navigation.dispatch(navigateToAccountDetails(user.user_id))
seems to work. But that doesn't help us shrink navActions
for #4417.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, both before and after #5397, I've had trouble with Flow not accepting a
useNavigation()
withnavigation.push('account-details', { userId })
, even though I think it should be fine. Do you see an easy way to do that?
Hm. Trying it, Flow gives the error that property push
is missing on the type of navigation
.
I think basically the issue is that useNavigation
returns a plain NavigationProp
, but we want specifically a StackNavigationProp
. That should be fixable in our type-wrapper.
Though the other thing about actually cutting out navActions
is that we don't have useful type-checking on those StackActions.push
calls, of the fact that the parameters they pass align with what the screens actually expect. Keeping them all centralized in one place is a way of mitigating that, by making it easier to more reliably check them by hand. See: #4757 (comment) (which has been on my to-do list to write up.)
Hmm, but I guess that problem might be solvable precisely by saying navigation.push
instead of StackActions.push
! Because the navigation
object should have a type tying it to our GlobalParamsList
which registers all those route-params types. I'll look into that.
Even excluding the latter point: if the goal is to eliminate dispatching of nav actions and instead use navigation.push
, and if we still needed a central set of functions to mitigate a lack of type-checking, we could still accomplish that. It's just that the central set of functions wouldn't be returning actions, and instead would have signatures like navigateToAccountDetails: (navigation: NavigationProp<…>, userId: UserId) => void
and would make the navigation.push
calls themselves.
Anyway: I think for this PR, I'll switch to useNavigation
but leave it as a navigation.dispatch
. Then as a followup I'll look into making useNavigation
return a StackNavigationProp
, and converting this and others to say navigation.push
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That followup may be helpful toward the react-navigation v6 upgrade #4936, too.)
923980c
to
4e22537
Compare
Now that the full profile as shown on AccountDetailsScreen contains not only the user's last-active time and their local time, but also their custom profile fields, it's increasingly likely that a user may want to check what their own profile looks like. Add a button to do so from ProfileScreen, i.e. from the tab in our main screen that has your own avatar as its icon.
4e22537
to
8209ee1
Compare
Such a nicer syntax! This is available to us now that we've upgraded Prettier: zulip#5393 (comment) and Flow before that: zulip#5398 (comment) We'll similarly convert $ElementType in the next commit. Done mostly with a crude search-and-replace: $ perl -i -0pe ' s/\$PropertyType<(.*?),\s*(.*?)>/${1}[$2]/gs ' src/**/*.js types/**.js.flow flow-typed/**/*.js That doesn't behave right in the presence of nesting; but there was only one case of that, so just fixed it up by hand.
Fixes: #2900
This implements for mobile a feature we've had in web for a while, which is to show people's custom profile fields when you're looking at their profile. Doing so also sets us up to pursue #5363, which is an in-development further wrinkle on that feature.
Here's some screenshots. (For the "before", just mentally erase the custom profile fields.) With a real profile on chat.zulip.org:
Following the link gives:
On the my-profile screen (the rightmost tab on the main screen):
(I don't love that placement between the name and the other UI; seems like that'd get annoying when you have a lot of fields set. I'll play around with that.)
On a test account with one of each type of custom profile field: