-
Notifications
You must be signed in to change notification settings - Fork 69
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
bug: add handler for handling changed domain #500
base: master
Are you sure you want to change the base?
Conversation
invenio_accounts/datastore.py
Outdated
@@ -73,11 +73,16 @@ def commit(self): | |||
|
|||
def mark_changed(self, sid, uid=None, rid=None, model=None): | |||
"""Save a user to the changed history.""" | |||
# needed so that we have the id from the DB on the model | |||
self.db.session.flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slint Can you have a look at the impact of having a flush here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we very often tiptoe around flushes, and get them in implicitly just by chance because we perform another SELECT
query down the line, e.g.:
domain = Domain.create(...)
# ...in another function...
user = UserAggregate.get(...) # <-- this triggered a flush, so our `domain`
# from above now has a `domain.id`
So I think it's better to be explicit about it, which is what calling mark_changed
also implies (i.e. that something will be persisted to the DB).
@carlinmack is also checking this change on tests across other repos to see if there are wrong expectations.
31ddca0
to
d37027a
Compare
remove setting of role.id as the added flush() will add the ID from DB
6f9ad3c
to
b8ae274
Compare
b8ae274
to
72e7cf1
Compare
❤️ Thank you for your contribution!
(Part of) Close inveniosoftware/invenio-app-rdm#2777
Description
We call
mark_changed
but we did not check that it handled DomainsCalling flush as similarly done in the pre-commit hook
Also remove setting of role.id as the added flush() will add the ID from DB
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: