Refactor Event creation to avoid two DB writes when creating Event with disavowal#7301
Merged
mitchellhenke merged 4 commits intomainfrom Nov 7, 2022
Merged
Conversation
zachmargolis
approved these changes
Nov 4, 2022
app/services/user_event_creator.rb
Outdated
Contributor
There was a problem hiding this comment.
i think rubocop will request a trailing comma?
Suggested change
| disavowal_token: disavowal_token | |
| disavowal_token: disavowal_token, |
app/services/user_event_creator.rb
Outdated
Contributor
There was a problem hiding this comment.
possibly same?
Suggested change
| disavowal_token: disavowal_token | |
| disavowal_token: disavowal_token, |
app/services/user_event_creator.rb
Outdated
Comment on lines
33
to
34
Contributor
There was a problem hiding this comment.
maybe could consider switching to keyword args and just inlining?
Suggested change
| disavowal_token = SecureRandom.urlsafe_base64(32) | |
| create_user_event(event_type, user, disavowal_token) | |
| create_user_event(event_type, user, disavowal_token: SecureRandom.urlsafe_base64(32)) |
mitchellhenke
force-pushed
the
mitchellhenke/save-an-update-on-events
branch
3 times, most recently
from
66736bf to
a111151
Compare
…th disavowal changelog: Internal, Database, Refactor Event creation to avoid two DB writes when creating Event with disavowal
mitchellhenke
force-pushed
the
mitchellhenke/save-an-update-on-events
branch
from
a111151 to
e6d2736
Compare
zachmargolis
reviewed
Nov 7, 2022
zachmargolis
reviewed
Nov 7, 2022
zachmargolis
reviewed
Nov 7, 2022
zachmargolis
reviewed
Nov 7, 2022
Comment on lines
+107
to
+108
| event.disavowal_token = disavowal_token | ||
| event |
Contributor
There was a problem hiding this comment.
maybe a bigger refactor than is good for this PR, but what if we removed the transient disavowal_token plaintext attribute and just returned it separately? ex
Suggested change
| event.disavowal_token = disavowal_token | |
| event | |
| [event, disavowal_token] |
Contributor
Author
There was a problem hiding this comment.
I think that makes sense to tackle following this, yeah
Contributor
Author
There was a problem hiding this comment.
It's also kind of a mess and could probably stand to be refactored again.
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
mitchellhenke
force-pushed
the
mitchellhenke/save-an-update-on-events
branch
from
9efc309 to
e756cf5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠 Summary of changes
Instead of creating an event and then updating a column, we can save one database write by creating the event with all of the columns at the beginning.