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

Main isn't required when updating an address #695

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

louis-lau
Copy link
Member

No description provided.

@NickOvt NickOvt self-requested a review June 4, 2024 20:00
@NickOvt
Copy link
Contributor

NickOvt commented Jun 5, 2024

Wondering whether that field is required at all in that endpoint 🤔.
Because even at lines 1051-1056 we can omit main and find out that the address is main by previously comparing the given address path param and fetched user data. Something like

main = result.value.id === userData.address && userData.address === addressData.address

@louis-lau
Copy link
Member Author

louis-lau commented Jun 5, 2024

What do you mean exactly? You'd want to be able to set main: true in the update address endpoint right?

As far as I know it wasn't required before. Since I can only set it to true. It makes it impossible to update an address without making it the main address.

@NickOvt
Copy link
Contributor

NickOvt commented Jun 5, 2024

Yeah it doesn't need to be required indeed. I think I incorrectly analyzed the endpoint before.
But what I was thinking with last comment is whether we need the main param at all? Like, the only values basically that it can get are undefined and true, because if it is false then we just stop and return an error (Cannot unset main status).

Copy link
Contributor

@NickOvt NickOvt left a comment

Choose a reason for hiding this comment

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

I'll merge the change right now. We can always do other changes with future PRs.

@NickOvt NickOvt merged commit c9188b3 into nodemailer:master Jun 5, 2024
4 checks passed
@louis-lau
Copy link
Member Author

But what I was thinking with last comment is whether we need the main param at all

Otherwise how do I set an address as the main one?

@louis-lau louis-lau deleted the fix-main-being-required branch June 5, 2024 10:34
@NickOvt
Copy link
Contributor

NickOvt commented Jun 5, 2024

What about lines 1051 - 1063? IF I understand correctly then the main value can actually be derived via an if statement. So basically if the path param id, which is the address, is equal to the email in the userData then it seems it is the main address, and if the new address is different then we set it as the new main.

But I guess actually having a field for main is clearer :D. So let's leave it like that right now

@louis-lau
Copy link
Member Author

and if the new address is different then we set it as the new main.

Why? If I have a non-main alias I want to change to another address, I don't nessecarily want it to become my new main address.

This is an endpoint for updating an existing address.

@louis-lau
Copy link
Member Author

And what if I had an existing alias that I wanted to make my main address? How could that be derived?

Sorry if I'm completely misunderstanding what you're saying

@NickOvt
Copy link
Contributor

NickOvt commented Jun 5, 2024

Right. It seems we need main for exactly this scenario. That we want to change a non-main address to be main. And that cannot be derived.

@louis-lau
Copy link
Member Author

Yeah exactly haha. Phew, I thought I was really missing something for a second there

@NickOvt
Copy link
Contributor

NickOvt commented Jun 5, 2024

Yeah I suppose my brain was actively ignoring the last if (!main) statement haha. So I thought that it could be derived, but nope, we need it exactly for the reasons of setting a non-main address to main. So all good! :D :D

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.

None yet

2 participants