Skip to content
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

Edit profile form for delegates #10456

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

danieljames-dj
Copy link
Member

This PR aims to open up the edit profile form for delegates, so that delegates can request for profile updates through form instead of sending email to WRT.

@danieljames-dj danieljames-dj force-pushed the edit-profile-delegate branch 2 times, most recently from dbed680 to 6fb26d9 Compare December 21, 2024 13:54
app/webpacker/components/ContactsPage/ContactForm.jsx Outdated Show resolved Hide resolved
);
}

export default function ContactEditProfilePage({ loggedInUserId, recaptchaPublicKey }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to have the main/exported component at the top, and the helper components below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay changed, I actually thought the reverse - like the exported component should be at bottom - just like how we have main method at bottom of programs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's often the case because in some languages anything that a main method references has to have already been defined, so you have to put it at the bottom. Which isn't the case with React.

The point of this file is to provide (export) a component, so the most useful thing to see first is that component. You can then scroll down further to see more details if you want.

But it's not a big deal.

Copy link
Member

@gregorbg gregorbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find a way to handle the frontend rendering without copy-pasta of the "self-edit or other-edit" component.

Backend is fine.

Comment on lines 56 to 60
const { data: loggedInUserData, isLoading, isError } = useQuery({
queryKey: ['userData'],
queryFn: () => fetchJsonOrError(apiV0Urls.users.me.userDetails),
}, CONTACT_EDIT_PROFILE_QUERY_CLIENT);
const wcaId = loggedInUserData?.data?.user?.wca_id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct that you're running this whole query just to get the current user's WCA ID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in future maybe we can have a hook like useLoggedInUserPermission to get the WCA ID to make it simpler.

Comment on lines 39 to 52
if (queryParams.editOthersProfile) {
return (
<ContactEditProfileOthers
setContactSuccess={setContactSuccess}
recaptchaPublicKey={recaptchaPublicKey}
/>
);
}
return (
<ContactEditProfileSelf
setContactSuccess={setContactSuccess}
recaptchaPublicKey={recaptchaPublicKey}
/>
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this duplication approach. It should definitely be possible to have just one ContactEditProfile component, and give it a WCA ID search input which either happens to be pre-filled (and disabled, in case of self-edit) or enabled and open for the user to search whathever ID they want to edit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I merged both of them. I split them because I feel they were bit complicated when together.

return (
<Container text>
<Header as="h2">{I18n.t('page.contact_edit_profile.title')}</Header>
<WcaSearch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In relation to my other comment above, we also have a IdWcaSearch component (in the same file that defined WcaSearch which is purely ID-based and may help you here, because it seems that person is a dummy value whose only purpose is to store a WCA ID which you later pass down to the edit component.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, done.

Copy link
Member

@gregorbg gregorbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Just a few nits left now.

return render status: :unauthorized, json: { error: "Cannot request profile change without login" } unless current_user.present?
if current_user.nil?
return render status: :unauthorized, json: { error: "Cannot request profile change without login" }
elsif current_user.person.wca_id != wca_id && !current_user.has_permission?(:can_request_to_edit_others_profile)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: You can access wca_id directly on current_user. In fact, the way that a User model even knows which Person it belongs to is precisely because it has an wca_id field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code snippet from user.rb:

belongs_to :person, -> { where(subId: 1) }, primary_key: "wca_id", foreign_key: "wca_id", optional: true

// If not logged in, fetching WCA ID of logged in user is not possible.
!!loggedInUserId
// If the user needs to edit other's profile, then fetching own WCA ID is not needed.
|| !editOthersProfileMode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to see thrrough this. I think a better explanation of the ! should be: If the user is not editing somebody else's profile, then we need to fetch their own WCA ID.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The problem is that your comment explains when the fetching should be DISabled, but the useQuery function needs to be passed information when it is ENabled. So I was confused why your comment says "if the user needs to edit other's profile", while the code directly below says "NOT editing other's profile")

@@ -47,7 +50,7 @@ export default function ContactForm({
const contactFormState = useStore();
const dispatch = useDispatch();
const { formValues: { contactRecipient: selectedContactRecipient, userData } } = contactFormState;
const formRedirection = useMemo(() => getFormRedirection(contactFormState.formValues));
const formRedirection = getFormRedirection(contactFormState.formValues);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you get rid of the useMemo? You should of course pass a dependency array correctly (probably my bad for letting that slip through in the initial review of this page) but otherwise I feel like formValues can change relatively often so memoing the result feels like a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested it in my review:

This calculation isn't expensive and the const isn't used in any dependency arrays, so I don't see any reason to memo this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's interesting! My understanding was always that memoing stuff is worth it if the inputs to the calculation change (semi-)frequently, not whether the output of the memo is then later used as a dependency down the line. Would you care to elaborate? I'm eager to learn more! 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vast majority of consts we use in components are very quick/easy to compute, so it doesn't really matter if they get recomputed frequently. In simple cases, memoing them might even (technically) slow things down, since React needs to look at the dependency array and check if anything has changed and also stores the old value even when it's not needed (but the slowing down would be negligible).

As far as performance goes, the more often the inputs change, the less useful memoizing is, because you use the cached value less frequently and need to recompute anyway.

Some of the first results from a google search about when to use useMemo:

https://maxrozen.com/understanding-when-use-usememo

don't use useMemo until you notice parts of your app are frustratingly slow.

https://blog.logrocket.com/when-not-to-use-usememo-react-hook/

Most methods on JavaScript data types are optimized, e.g. Array.map, Object.getOwnPropertyNames(), etc. If you’re performing an operation that’s not expensive (think Big O notation), then you don’t need to memoize the return value. The cost of using useMemo may outweigh the cost of reevaluating the function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And hopefully it's clear that memoing is (potentially) helpful when a calculation is expensive; and that if you're using an array/object/function in another dependency array then it needs memoing otherwise that other dependency array would trigger an update every single render.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants