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

[WIP] Add access to query builder with requests() and friendships() method in friendable trait #16

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

stephane-monnot
Copy link
Contributor

No description provided.

@stephane-monnot
Copy link
Contributor Author

I have a question, method getBlockedFriendships() should return all blocked friendships ? hasBlocked and isBlockedBy ? Same question for getAcceptedFriendships()

@hootlex
Copy link
Owner

hootlex commented Mar 7, 2016

All of getBlockedFriendships, getAcceptedFriendships, getDeniedFriendships, and getPendingFriendships return incoming and outgoing friendships.

@barryvdh
Copy link

barryvdh commented Aug 5, 2016

This PR seems to do a bit much, but it would be nice to be able to query the friendships and add own scopes/filters/orders. So get the query builder instead of the fetched collection.

@hootlex
Copy link
Owner

hootlex commented Aug 8, 2016

@shinbuntu @barryvdh Yeah, accessing the query builder would make the package much more flexible.
This PR is based on this discussion but seems to be abandoned. :/

The basic idea is that:

// returns the query builder
$user->friendships(); 

// query requests from others users than the current
$user->friendships()->incoming(); 

// returns all incoming friendships
$user->friendships()->incoming()->get(); 

@barryvdh
Copy link

barryvdh commented Aug 8, 2016

Hmm, but doesn't the morphMany work one-way? Eg. only the friendships where the current user is the sender, not the other way around?

@barryvdh
Copy link

barryvdh commented Aug 8, 2016

Oh friendships are 'requests' and friends are connected friends, right?
Why not call it friendshipRequest instead of friendship? Imho the difference between friends and friendships is not so clear.

@hootlex
Copy link
Owner

hootlex commented Aug 9, 2016

@barryvdh Yup, it's actually too confusing. I think that the idea is that friendships() will return all friendships (pending, accepted, rejected, etc) and friends() will return the User models (ex $user->friends()->blocked() returns all the Users that have been blocked).

I can't decide how to name the methods/scopes in the new version of the API.
Maybe something like $user->friendships()->accepted()->getFriends() and remove friends() entirely.

Do you have anything to suggest?

@barryvdh
Copy link

barryvdh commented Aug 9, 2016

I think friends /friendships or connections when accepted. Requests can be all. $user->requests()->pending(). Or more specific friendRequests()

@stephane-monnot
Copy link
Contributor Author

stephane-monnot commented Aug 9, 2016

Both "friends" method and "requests" method will return a queryBuilder to get a collection of users ? Or "requests" method will return a queryBuilder to get a collection of FriendshipRequest (Actually Friendship model) ?

@hootlex
Copy link
Owner

hootlex commented Aug 9, 2016

Hmm, maybe we can keep only friendRequests or friendships which will return User Models. I don't think there is a need to return Friendship models

What do you think about that?

@stephane-monnot
Copy link
Contributor Author

stephane-monnot commented Aug 9, 2016

I added friendships() to get friends querybuilder, deprecated friends method and added protected friendshipRequests to keep morphMany relationship with Friendship Model.

For "$user->requests()->pending()", the requests should be incoming only or incoming+outgoing ?

@hootlex
Copy link
Owner

hootlex commented Aug 9, 2016

I liked your idea with the incoming and outgoing scopes.
If we keep them, then $user->requests()->pending() should return all pending requests and $user->requests()->incoming()->pending() should return only the incoming ones.

@stephane-monnot
Copy link
Contributor Author

I added some scopes, but I have encountered some difficulties with outgoing and incoming scopes because I can't get User object in the scopes.

Maybe a solution consists to add named scopes and remove one scope to filter. (See where clauses in findRequests)

Examples:

  • defaultScope1: orWhereSender($this)
  • defaultScope2: orWhereRecipient($this)

When I call outgoing(), it will remove scope2 and add scope1.
When I call incoming(), it will remove scope1 and add scope2.

Better solution ?

@hootlex
Copy link
Owner

hootlex commented Aug 10, 2016

I don't know if there is a better way to get the User. I can review it tomorrow and let you know if I find a better solution.

@stephane-monnot
Copy link
Contributor Author

Thank you. I will check tomorrow night.

@hootlex
Copy link
Owner

hootlex commented Aug 13, 2016

I couldn't find any way to get user withing incoming/outgoing scopes.
One thing that we could to, is to make incoming/outgoing functions withing Friendable trait.
Also, your solution sounds good to me.

@stephane-monnot stephane-monnot changed the title [WIP] Add access to query builder with friends() and friendships() method in friendable trait [WIP] Add access to query builder with requests() and friendships() method in friendable trait Aug 15, 2016
@hootlex
Copy link
Owner

hootlex commented Aug 17, 2016

@stephane-monnot Let me know when you are done.

@stephane-monnot
Copy link
Contributor Author

You can already check if you have time. You can quickly see changes in this commit.

Maybe we should deprecate some methods, update the documentation and make a deprecated section with all deprecated methods.

What do you think about that? Do you have any suggestions ?

@hootlex
Copy link
Owner

hootlex commented Aug 18, 2016

Looks awesome! Though, Direction::INCOMING in $user->requests(Direction::INCOMING) is kinda scary...
I am thinking to bump version to 2.0 after I merge this.

Do you prefer requests as the public method over friendRequests?

@barryvdh
Copy link

To be honest, I liked the incoming() scope more then this constant.

@stephane-monnot
Copy link
Contributor Author

stephane-monnot commented Aug 18, 2016

Me too. I will try to find a solution.

@hootlex
Copy link
Owner

hootlex commented Sep 14, 2016

@stephane-monnot I can't wait to write proper docs for V2.

@stephane-monnot
Copy link
Contributor Author

Sorry, I have a lot of work. I will work on it soon.

@hootlex
Copy link
Owner

hootlex commented Sep 14, 2016

@stephane-monnot sure, no problem. Just wanted to let u know 😄

@nikolaynesov nikolaynesov mentioned this pull request Sep 29, 2016
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.

None yet

3 participants