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

fix custom notification for deleted event #451

Merged
merged 1 commit into from Apr 30, 2021
Merged

Conversation

jeriox
Copy link
Contributor

@jeriox jeriox commented Apr 28, 2021

No description provided.

@jeriox jeriox requested a review from felixrindt April 28, 2021 18:54
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 89.431% when pulling f6602cc on bugfix_notifications into dcf316e on main.

data=dict(email=participant.email, event_id=event.id, content=content),
data=dict(
email=participant.email,
event_id=event.id,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the id, still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not at the moment, but I think we should keep the ability to fetch the event from the db for sending to query for more information (if it exists)

email=participant.email,
event_id=event.id,
content=content,
event_title=event.title,
Copy link
Member

Choose a reason for hiding this comment

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

I must admit this feels like fixing symptoms. There are more occasions in which we will try to fetch objects in Notification sending that have been deleted in the meantime...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well usually you won't confirm people directly before deleting the event, so this will be the case where it happens IMO.
What would be your suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

I think the best solution would be to send Notifications without delay, but that's for another day.

email=participant.email,
event_id=event.id,
content=content,
event_title=event.title,
Copy link
Member

Choose a reason for hiding this comment

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

I think the best solution would be to send Notifications without delay, but that's for another day.

@jeriox jeriox merged commit 873ab71 into main Apr 30, 2021
@jeriox jeriox deleted the bugfix_notifications branch April 30, 2021 19:56
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

3 participants