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
Refactor subject model #2070
Refactor subject model #2070
Conversation
14327c4
to
655566f
Compare
Looking good @AndyStabler 👍 |
ebc7cce
to
a27ff97
Compare
sync_involved_users if (saved_changes.keys & notifiable_fields).any? | ||
end | ||
|
||
def self.sync(remote_subject) |
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.
How do you feel about having an instance and a class method with the same name? We can change this to something else if we like I'm just wary of changing too much in one PR.
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 don't mind that, I've done it a lot in the past
Lots of the stuff going on inside the Subject.sync method is acting on an instance and not doing things at the class level. This commit extracts a Subject#sync instance method to make things more object oriented and to allow us to start extracting the logic out of this method. This should hopefully make things easier to understand what's going on.
a27ff97
to
fac7705
Compare
Thanks for taking a look @andrew 🙇 I've cleaned things up and it's ready for review! |
I started a refactor of the subject class aages ago and I'm finally getting round to looking at it again.
I'd like to poke the
sync
method to see what it looks like when it's working on an instance ofsubject
instead of operating at a class level.