-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Implementation/47064 support custom field in the attributesbytimestamp #13158
Implementation/47064 support custom field in the attributesbytimestamp #13158
Conversation
c1e1be4
to
0e1ec97
Compare
…lse to nil and vice versa.
c77b6b6
to
a371ddf
Compare
…-the-attributesbytimestamp
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.
Hey Attila, I like the change. The number of places touched is really kept in check nicely.
I have two tiny remarks in the code and one issue that can also be addressed in a subsequent PR. That is actually what I would suggest, to fix the two small issues and have the bigger issue addressed separately.
The bigger issue is an N+1 query when the diff for the representer is created:
We also have https://community.openproject.org/wp/48572 where we might place the fix in. It is not strictly the same but related.
end | ||
end | ||
|
||
def no_nil_to_empty_strings?(normalized_old_data, attribute, new_value) | ||
old_value = normalized_old_data[attribute] | ||
new_value != old_value && (new_value.present? || old_value.present?) | ||
new_value != old_value |
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 thinks this method no longer serves it's purpose of checking whether value might have changed from e.g. nil
to ''
as the method name suggests.
new_value = nil
old_value = ''
new_value != old_value && (new_value.present? || old_value.present?)
=> false
new_value != old_value
=> true
To have the implementation be more explicit, one could change it into
([new_value, old_value] - ['', nil]).any?
Update note In an earlier version of this old_value = nil
was specified which was incorrect.
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.
It is unsettling that no spec pics up on this. So I don't know how relevant my complaint is.
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 made the changes here 953bdf0. The expectations now fail in case any of the nil -> ''
attribute transitions are included.
Some minor adjustment was required on the conditions, because [false].any?
returns false
and it breaks the expectation when a transition happens from nil -> false
. I used ([new_value, old_value] - ['', nil]).present?
instead.
Journal | ||
.where(id: journal_ids) | ||
.includes({ customizable_journals: :custom_field }) | ||
.index_by(&:id) |
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.
Tiny thing: The Journal
needn't be involved in the loading. The same result can be achieved by
Journal::CustomizableJournal
.where(journal_id: journal_ids)
.includes(:custom_field)
.index_by(&:journal_id)
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.
Done 57c3bf0
Thanks for the thorough review @ulferts. I addressed your remarks except the N+1 issue, that will be addressed separately. From my side this PR is good to go, once the CI is green. |
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.
👍 merge at will
https://community.openproject.org/wp/47064