-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add project and user for member #13
base: main
Are you sure you want to change the base?
Conversation
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.
While technically everything works from my point of view, I left some change hints regarding the new method. Maybe restructuring it would make it a bit easier to understand.
Also, can we enhance the unit test for this change?
attributes[:additional_data] = get_additional_data(subject, attributes[:additional_data]) | ||
if attributes[:additional_data] && attributes[:additional_data].key?(:member_id) | ||
attributes[:additional_data] = Hash(attributes[:additional_data]).merge(get_additional_data(subject.member, attributes[:additional_data])) |
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.
Some things:
- I find this method call a bit misleading, I'd rather call it
set_additional_data
. - I'd move this entire additional_data definition in
set_additional_data
method or even include the additional_data setting in this method. These are class methods, it's weird having those mixed in the private section of this class.
|
||
private | ||
|
||
def self.get_additional_data(subject, additional_data) |
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.
See my comment above regarding naming and content of this method.
shot2022-09-29.at.07.18.08.mp4