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

An enhanced/updated PR of #210 to fix messenger between scope in issue #209 #359

Merged
merged 11 commits into from
Nov 2, 2021

Conversation

AbdullahFaqeir
Copy link
Contributor

Issue Reference

#209

Description

between() scope behaves incorrectly, when asking to return the only thread between users (1, 2), it returns all the conversations which both of those users participate in.

So now, between() scope, keeps it's incorrect behaviour, yet now it's not incorrect for the case it's now supposed to do, which is returning any threads which the passed participants are in, but added a new betweenOnly() scope, which will only return the thread of which the passed participants are in.

Note : check test cases to understand more what happened.

How To Test This?

Run unit testing

Documentation

  • Added scope betweenOnly() to get private conversations between only passed participants.
  • Added test case for betweenOnly() scope.
  • Code refactor.
  • Fix typos.

… passed participants.

- Added test case for betweenOnly() scope.
- Code refactor.
- Fix typos.
src/Models/Message.php Outdated Show resolved Hide resolved
@AbdullahFaqeir AbdullahFaqeir changed the title An enhanced/updated PR of #2010 to fix messenger between scope. An enhanced/updated PR of #210 to fix messenger between scope. Oct 12, 2021
@AbdullahFaqeir
Copy link
Contributor Author

@cmgmyr can you please review this PR?

@AbdullahFaqeir
Copy link
Contributor Author

@cmgmyr I'm sorry for the nag ^^', but I need this package, if you can check the PR.

@AbdullahFaqeir AbdullahFaqeir changed the title An enhanced/updated PR of #210 to fix messenger between scope. An enhanced/updated PR of #210 to fix messenger between scope in issue #209 Oct 18, 2021
Copy link
Owner

@cmgmyr cmgmyr left a comment

Choose a reason for hiding this comment

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

@AbdullahFaqeir thank you for the contribution!

This looks good to me so far. Can you tell me if you tested this with PostgreSQL? I remember having issues with the query in my original PR. Ideally, we'd be able to support all of the drivers in Laravel.

@cmgmyr cmgmyr linked an issue Oct 19, 2021 that may be closed by this pull request
@AbdullahFaqeir
Copy link
Contributor Author

@cmgmyr I haven't actually test it on PostgreSQL, I'll do so and update you.

@AbdullahFaqeir
Copy link
Contributor Author

Hey @cmgmyr!

Well, finally this is solved once and for all.

According to (this )[https://www.postgresqltutorial.com/postgresql-having/]

public function scopeBetween(Builder $query, array $participants)
{
return $query->whereHas('participants', function (Builder $q) use ($participants) {
$q->whereIn('user_id', $participants)
->select($this->getConnection()->raw('DISTINCT(thread_id)'))
->groupBy('thread_id')
->havingRaw('COUNT(thread_id)=' . count($participants));
});
}

The way old between() scope was written in a way PostgreSQL won't be able to execute according to the cycle of evaluation of the SQL statement, as you can see, in the link above, the HAVING can't have access to aliases, as well column used in it must be used in the group clause.

After rewriting the SQL statement in the most generic way, I managed to make it work on PostgreSQL as well.

Tested, but couldn't commit the testing updates as they were really messed up; Tests configuration needs to be restructured in a proper way.

I believe this is ready to be merged now.

@cmgmyr
Copy link
Owner

cmgmyr commented Nov 1, 2021

@AbdullahFaqeir I updated the test suite to use GitHub actions as well as some other updates. Can you rebase/update and re-push your work? This should now catch any issues with MySQL and Postgres. Please let me know if you need any assistance.

src/Models/Thread.php Show resolved Hide resolved
@cmgmyr
Copy link
Owner

cmgmyr commented Nov 2, 2021

Perfect! I thought I saw those tests before - thanks for adding those back in and thanks so much for this PR! It's great to have this finally cleaned up!

I'll go ahead and merge this, but will wait to tag until next week. In the meantime, composer can be updated to reference dev-master for additional testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messenger Between Scope
3 participants