search

cmgmyr / laravel-messenger

Simple user messaging package for Laravel
MIT License
2.45k stars 514 forks source link

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

Closed AbdullahFaqeir closed 2 years ago

AbdullahFaqeir commented 2 years ago

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

AbdullahFaqeir commented 2 years ago

@cmgmyr can you please review this PR?

AbdullahFaqeir commented 2 years ago

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

AbdullahFaqeir commented 2 years ago

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

AbdullahFaqeir commented 2 years ago

Hey @cmgmyr!

Well, finally this is solved once and for all.

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

https://github.com/cmgmyr/laravel-messenger/blob/9d6b5443792ee573e4f229206b0ac323395bc3c3/src/Models/Thread.php#L206-L214

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 commented 2 years ago

@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.

cmgmyr commented 2 years ago

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