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

Messenger Between Scope #209

Closed
trotsaleksandrov opened this issue Feb 24, 2017 · 18 comments · Fixed by #359
Closed

Messenger Between Scope #209

trotsaleksandrov opened this issue Feb 24, 2017 · 18 comments · Fixed by #359

Comments

@trotsaleksandrov
Copy link

trotsaleksandrov commented Feb 24, 2017

Hello!
It appears for me, while trying to use default betweenScope located in Thread model, it doesn't work as expected. For example, Thread::between([1,2]) will return all the Threads where users with ids 1 and 2 are participating. Instead, it should return the only chat with users with these ids. Am i the one who is facing this issue? Can you provide some ideas, how to manage this?
Thanks in advance!

@antonkomarev
Copy link
Contributor

Did you mean that it returns threads where participants are: 1, 2, 3 too?

@trotsaleksandrov
Copy link
Author

Exactly

@antonkomarev
Copy link
Contributor

antonkomarev commented Feb 24, 2017

@trotsaleksandrov yeah... I've met such issue and just overrided a logic of getting the threads. You are free to extend Thread model with your own and write you own logic.

Change thread model in config.

I don't know is this planned behavior or not.

@trotsaleksandrov
Copy link
Author

That's the problem, that's it's hard to get the threads between exact users. But anyway, thanks for help :)

@antonkomarev
Copy link
Contributor

antonkomarev commented Feb 24, 2017

You can have additional column in threads table where all participants will be stored as string: 1,2,3
On search you can compare participants you want with this string.

The only thing you should to remember - always store ordered ids, because if you will save value as 1,3,2 this trick wouldn't work. And then you will need to have an index on this column.

@antonkomarev
Copy link
Contributor

antonkomarev commented Feb 25, 2017

But anyway @cmgmyr should look at this issue. As for me - current behavior is bugged.

Or maybe there should be one more additional method: betweenOnly. Which will return threads where only provided users are participating.

@trotsaleksandrov
Copy link
Author

Cannot but agree with you. It would be great if this functionality would be reviewed

@cmgmyr
Copy link
Owner

cmgmyr commented Feb 25, 2017

I started to look into this yesterday but wasn't able to get far before I had to get going. I am seeing the same thing on my end and agree that it should be fixed. I'll work on it.

I'm thinking that the current functionality could be useful for some. So maybe the between scope should return the threads only between users 1 and 2 (as intended), but adding something like betweenLoose that would return threads that include users 1 and 2, but also can have other participants (like current functionality). Thoughts?

@cmgmyr cmgmyr added the bug label Feb 25, 2017
@trotsaleksandrov
Copy link
Author

Maybe it is better to split existing between method to two: as you mentioned, something like betweenLoose and as @a-komarev mentioned betweenOnly. Because retrieving threads that includes users 1 and 2, might also be useful

@antonkomarev
Copy link
Contributor

antonkomarev commented Feb 25, 2017

@cmgmyr To make method betweenLoose was my first thought... but this will be breaking change.

We can add 2 new methods. betweenLoose and betweenOnly. Make method between as an alias for betweenLoose for now and mark it as @deprecated in docblock. In future major release it could be dropped or will be an alias for betweenOnly method. By this way we wouldn't affect already launched applications. Or just make major release :}

@cmgmyr
Copy link
Owner

cmgmyr commented Feb 25, 2017

@a-komarev yup, that's exactly what I was thinking. I've also been thinking about rewriting the whole package to be a little more user friendly and not just a bunch of models.

@antonkomarev
Copy link
Contributor

@cmgmyr Any ETA of these plans?

@cmgmyr
Copy link
Owner

cmgmyr commented May 1, 2017

@a-komarev the v3 plans? Unfortunately, not yet besides some planning in my head. Started a new job recently, so it's been cutting in on my free time a bit. Hopefully will get some code out soon though

@s1lviu
Copy link

s1lviu commented Sep 7, 2017

For me
$thread = Thread::between([1, 2])->latest('updated_at')->firstOrFail();
returns
No query results for model [Cmgmyr\Messenger\Models\Thread].

@cmgmyr cmgmyr linked a pull request May 28, 2020 that will close this issue
@AbdullahFaqeir
Copy link
Contributor

Still need help with this??

@cmgmyr
Copy link
Owner

cmgmyr commented Oct 11, 2021

@AbdullahFaqeir yes, this still needs to be worked on. I haven't gotten a chance to revisit it yet. Feel free to use the WIP as a starting point or just start fresh 👍

@AbdullahFaqeir
Copy link
Contributor

@cmgmyr working on it now.

@AbdullahFaqeir
Copy link
Contributor

@AbdullahFaqeir yes, this still needs to be worked on. I haven't gotten a chance to revisit it yet. Feel free to use the WIP as a starting point or just start fresh 👍

I've submitted a PR if you can review it.

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