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

Fixed longstanding bug with with_read_marks_for #56

Closed
wants to merge 4 commits into from

Conversation

mikeki
Copy link
Contributor

@mikeki mikeki commented May 26, 2015

There was a bug explained with the next code snippets:
Suppose:

message1 = Message.create!
user1 = User.create!
user2 = User.create!
message1.mark_as_read! :for => user1

expect behavior:

messages = Message.with_read_marks_for(user1)
# => [ message1 ] with read_mark_id
messages[0].unread?(user1) #check message1, if is unread? by user1
# => false
messages[0].unread?(user2) #check message1, if is unread? by user2
# => true, expect user2 to have unread message1

current behavior:

messages = Message.with_read_marks_for(user1)
# => [ message1 ] with read_mark_id
messages[0].unread?(user1) #check message1, if is unread? by user1
# => false
messages[0].unread?(user2) #check message1, if is unread? by user2
# => false ===>BUG<===

This happens because with_read_marks_for just joins with the read_mark_id, but there is never any indication as to who owns the read_mark_id, so currently we just assume it belongs to all readables

The same bug was implemented in my previous branch for readers(User), but gets fixed here too.

Fix:

I added read_mark_user_id for readables scope and read_mark_readable_type and read_mark_readable_id for reader scopes.

Now inside unread? and have_read?, besides testing if the record respond_to?(:read_mark_id) we also test if the read_mark_id belongs to the user/readable being tested, otherwise it would load the whole set of unread_by(user)/have_not_read(readable) and verify if the object that we are looking for is present.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.47% when pulling aebae63 on mikeki:read_marks_scoped_to_owner into 29dc7c2 on ledermann:master.

private

def read_mark_id_belongs_to?(readable)
self.read_mark_readable_type == readable.class.name and
Copy link
Owner

Choose a reason for hiding this comment

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

Small bug: Must be compared to readable.class.base_class.name. Also, I would prefer to use && over and and || over or.

@ledermann
Copy link
Owner

Great description and fix! Thanks @mikeki, will merge soon!

@ledermann ledermann self-assigned this May 26, 2015
@mikeki
Copy link
Contributor Author

mikeki commented May 26, 2015

Addressed your comments.

Thank you :)
When do you plan on releasing a new version?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.54%) to 100.0% when pulling 613fd67 on mikeki:read_marks_scoped_to_owner into 29dc7c2 on ledermann:master.

@ledermann
Copy link
Owner

Squashed and merged into 11a2547.
I'm planning a new release for tomorrow. Again, thank you very much for contribution!

@ledermann ledermann closed this May 26, 2015
@mikeki mikeki deleted the read_marks_scoped_to_owner branch May 26, 2015 17:49
@mikeki
Copy link
Contributor Author

mikeki commented May 26, 2015

Thank you, and don't worry!

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.

3 participants