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

Prevent new users/members to be stored in db when invite fails #5350

Merged

Conversation

BlackDex
Copy link
Collaborator

@BlackDex BlackDex commented Jan 5, 2025

Currently when a (new) user gets invited as a member to an org, and SMTP is enabled, but sending the invite fails, the user is still created.
They will only not have received a mail, and admins/owners need to re-invite the member again.
Since the dialog window still keeps on-top when this fails, it kinda invites to click try again, but that will fail in mentioning the user is already a member.

To prevent this weird flow, this commit will delete the user, invite and member if sending the mail failed.
This allows the inviter to try again if there was a temporary hiccup for example, or contact the server admin and does not leave stray users/members around.

Fixes #5349

@stefan0xC
Copy link
Contributor

What about the CollectionUser records? Wouldn't they need to be deferred as well?

@BlackDex
Copy link
Collaborator Author

BlackDex commented Jan 5, 2025

What about the CollectionUser records? Wouldn't they need to be deferred as well?

Ah yes that needs to be moved to.

@BlackDex
Copy link
Collaborator Author

BlackDex commented Jan 6, 2025

I also wonder if this is the right way to go btw.
Now i defer saving the objects, but that could also mean, that after sending an email, storing the objects might fail, which i think is far worse then not being able to sent out an email.

I think it's better to save the objects, and upon mail failure, delete the items.

The only thing i would then put after sending an email is adding the user to the groups and collections.

@BlackDex BlackDex force-pushed the defer-member-save-after-mail-sent branch from 3ff5bf7 to d0a49b9 Compare January 6, 2025 20:29
@BlackDex BlackDex changed the title WIP: Defer creating users until after mail Prevent new users/members to be stored in db when invite fails Jan 6, 2025
@BlackDex BlackDex marked this pull request as ready for review January 6, 2025 20:30
@BlackDex BlackDex requested a review from dani-garcia January 6, 2025 20:30
@BlackDex
Copy link
Collaborator Author

BlackDex commented Jan 6, 2025

Ok, I made some changes. Instead of deferring, I now delete the records if sending the mail fails.
While this will cause some extra db queries to be executed, I do not think this will happen that much, and still keeps things more as they were.

I did moved creating the groups and collection records to after sending the mail, as those need the other records anyway.

@BlackDex BlackDex requested a review from stefan0xC January 7, 2025 10:45
Copy link
Contributor

@stefan0xC stefan0xC left a comment

Choose a reason for hiding this comment

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

Looks good to me. Even if it does not prevent sending mails to non-existent hosts.

Not sure if that is something that we can fix because that's something that happens after the mail has been successfully received by the MTA

an 07 12:38:23 mx postfix/lmtp[2296669]: 8573C4078F: to=<[email protected]>, relay=127.0.0.1[127.0.0.1]:10024, delay=5.1, delays=0.28/0.1/0.02/4.7, dsn=2.0.0, status=sent (250 2.0.0 from MTA(smtp:[127.0.0.1]:10025): status=sent (250 2.0.0 from MTA(smtp:[127.0.0.1]:10025): 250 2.0.0 Ok: queued as 5703C40709
an 07 12:38:23 mx postfix/qmgr[1824492]: 8573C4078F: removed
an 07 12:38:23 mx postfix/smtp[2296676]: 5703C40709: to=<[email protected]>, relay=none, delay=0.36, delays=0.09/0.25/0.02/0, dsn=5.1.0, status=bounced (Domain example.com does not accept mail (nullMX))

@BlackDex
Copy link
Collaborator Author

BlackDex commented Jan 7, 2025

That will be difficult indeed, also, sometimes MTA's use graylogging, which could cause these kind of messages.
Thanks for testing and reviewing!

Currently when a (new) user gets invited as a member to an org, and SMTP is enabled, but sending the invite fails, the user is still created.
They will only not have received a mail, and admins/owners need to re-invite the member again.
Since the dialog window still keeps on-top when this fails, it kinda invites to click try again, but that will fail in mentioning the user is already a member.

To prevent this weird flow, this commit will delete the user, invite and member if sending the mail failed.
This allows the inviter to try again if there was a temporary hiccup for example, or contact the server admin and does not leave stray users/members around.

Fixes dani-garcia#5349

Signed-off-by: BlackDex <[email protected]>
Signed-off-by: BlackDex <[email protected]>
@BlackDex BlackDex force-pushed the defer-member-save-after-mail-sent branch from 6056005 to f4130f7 Compare January 7, 2025 11:51
@dani-garcia dani-garcia merged commit 86aaf27 into dani-garcia:main Jan 8, 2025
5 checks passed
@BlackDex BlackDex deleted the defer-member-save-after-mail-sent branch January 9, 2025 07:38
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.

New member shows invited when the email was not sent
3 participants