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

Fix notification for new conversations #5686

Merged
merged 1 commit into from Feb 19, 2015

Conversation

@svbergerem
Copy link
Member

commented Feb 19, 2015

Fixes #5330, maybe introduced by this commit. The spec I posted in #5330 wasn't working but I wrote a cucumber feature.

@svbergerem svbergerem force-pushed the svbergerem:fix-conversations-notification branch 2 times, most recently from a230e59 to c661d4e Feb 19, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

@svbergerem hm, if the commit you linked is the problem, maybe we have other regression elsewhere.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

Mmh, you mean that self.attributes returns something that makes it always unique or never matching?

Nvm, I see the mistake.

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2015

@jhass Here is an example of self.attributes:
{"id"=>nil, "subject"=>"test9", "guid"=>"30cbe7809a5e0132c4c9047d7b8eb256", "author_id"=>1, "created_at"=>Thu, 19 Feb 2015 12:10:17 UTC +00:00, "updated_at"=>nil}

I guess there is no object with the same attributes in the db so we try to create a new one which fails because there is already a conversation with the same guid in the db.

@@ -86,7 +86,7 @@ def subscribers(user)
end

def receive(user, person)
cnv = Conversation.find_or_create_by!(self.attributes)
cnv = Conversation.create_with(self.attributes).find_or_create_by!(self.attributes[:guid])

This comment has been minimized.

Copy link
@jhass

jhass Feb 19, 2015

Member

Doesn't this have to be ..find_or_create_by!(guid: guid) then? This just seems to insert a WHERE 123456abcdef into the query if I'm following correctly.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 19, 2015

Author Member

Already tried that. In that case we create a second conversation in the cukes which makes them fail:

This comment has been minimized.

Copy link
@jhass

jhass Feb 19, 2015

Member

Just checked, it generates the following query:

SELECT  "conversations".* FROM "conversations"  LIMIT 1

That just happens to match in the test.

This comment has been minimized.

Copy link
@jhass

jhass Feb 19, 2015

Member
Conversation.create_with(attributes).find_or_create_by!(guid: guid)

generates:

SELECT  "conversations".* FROM "conversations"  WHERE "conversations"."guid" = 'cf7cd2e09a6001329fdd14feb5c40cd3' LIMIT 1

and doesn't fail the test for me.

@svbergerem svbergerem force-pushed the svbergerem:fix-conversations-notification branch from c661d4e to 53714c4 Feb 19, 2015

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2015

@Flaburgan Of course we have other regressions somewhere else but I don't think that the linked commit caused them. :-P

All other changes in the commit seem legit.

@jhass jhass added this to the next-major milestone Feb 19, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

Alright, good catch! Thank you!

jhass added a commit that referenced this pull request Feb 19, 2015

Merge pull request #5686 from svbergerem/fix-conversations-notification
Fix notification for new conversations

@jhass jhass merged commit fb1ff06 into diaspora:develop Feb 19, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound Hound has reviewed the changes.

@svbergerem svbergerem deleted the svbergerem:fix-conversations-notification branch Feb 19, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

Thank you Steffen! This bug was so annoying!

@marienfressinaud

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2015

I strongly agree, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.