Skip to content
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

make user relation polymorphic #63

Merged
merged 3 commits into from
Oct 27, 2015

Conversation

everplays
Copy link
Contributor

fixes #33.

@everplays
Copy link
Contributor Author

@ledermann I didn't add new tests as the test coverage is pretty good but let me know if you like to have some explicit testing for polymorphic user.

@@ -27,8 +27,9 @@ def mark_array_as_read(array, user)
if global_timestamp && global_timestamp >= timestamp
# The object is implicitly marked as read, so there is nothing to do
else
rm = obj.read_marks.where(:user_id => user.id).first || obj.read_marks.build
rm = obj.read_marks.where(:user_id => user.id, :user_id => user.class.base_class.name).first || obj.read_marks.build
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... this must be :user_type =>

@ledermann
Copy link
Owner

Thank you @everplays, great PR! Some additional ideas:

  • To improve naming things, we should rename the polymorphic database column from user to reader, so we have reader_id and reader_type. This naming would better match to acts_as_reader
  • We should add a migration to support converting existing read_marks
  • We should add some tests handling two different reader classes (because they are missing yet, the typo above does not break the tests)

@everplays
Copy link
Contributor Author

@ledermann sure. I can make these changes. However, changing user to reader will break all existing codes. Are you sure that we should do that?

@everplays
Copy link
Contributor Author

@ledermann I've applied two of your comments (out of four). Let me know if you want to break BC so I can add the migration and change user to reader. Otherwise, I'll add the migration and this should be good to merge.

@ledermann
Copy link
Owner

Fine!

But as I see it we are already introducing breaking changes, because the database schema requires the new column user_type. Without a migration the gem will not work with old data.

In addition, if a developer is using the public API of the gem only, changing some column names in the read_marks table will not break existing code (except for the needed migration) - or am I wrong?

In a nutshell, I prefer to rename the column names.

@everplays
Copy link
Contributor Author

@ledermann done. I've added some documentation as well. Feel free to remove/alter them.

@ledermann
Copy link
Owner

Great, thank you very much @everplays! Will merge it soon...

@ledermann ledermann merged commit 327aa20 into ledermann:master Oct 27, 2015
@everplays everplays deleted the making-user-polymorphic branch October 27, 2015 12:31
ledermann added a commit that referenced this pull request Oct 30, 2015
We need to iterate through all defined reader classes
Belongs to #63
ledermann added a commit that referenced this pull request Oct 30, 2015
ledermann added a commit that referenced this pull request Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to set multiple models as readers
2 participants