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

Milestone 5 friendshipv1 #10

Merged
merged 10 commits into from
Oct 3, 2019
Merged

Milestone 5 friendshipv1 #10

merged 10 commits into from
Oct 3, 2019

Conversation

samgaco
Copy link
Owner

@samgaco samgaco commented Oct 1, 2019

Milestone 1

#2

Milestone 2

#4

Milestone 3

#5
(it got finally approved after a rectification by the last TSE to review in that PR)

Milestone 4

#9


Milestone 5

  • Created a model with associations and all requested features for friendships.
  • Remember about unit and integrations tests!

Rubocop doesn't get activated in the pull request:

I am running the linters locally because of the reasons mentioned in my last comment in milestone 2 ( #4 ).

The offenses remaining I cannot solve them, they all concern the same thing and researching it seems like they could be bugs


Working alone:

I am working on this project alone, allowed by student success.

Copy link

@mosaaleb mosaaleb left a comment

Choose a reason for hiding this comment

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

Well done @samgaco 👏

  • sender sent a friend request to receiver. reciever can't send a friend request to sender.

friendship

We can achieve this by adding interchangeable validation to check if the [user_id, friend_id] or [friend_id, user_id] exist in the friendship table. please take a look at this example

  • [OPTIONAL] Previous step adds model validations to friendships uniqueness. I would suggest to add interchangeable DB validations. You need to know first why DB validations are important why.
    My previous code partner wrote an article on how to achieve this.

  • Please see my comments below.

Please handle these requirements and submit a new code review.
Happy Coding 💻


has_many :friendships, lambda { |user|
unscope(:where).where('user_id = :id OR friend_id = :id', id: user.id)
}, class_name: :Friendship, dependent: :destroy
Copy link

Choose a reason for hiding this comment

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

That's perfect way to handle friendships and inverse friendships 👏


# scopes
scope :already, ->(user, friend) { where('user_id = (?)', user).where('friend_id = (?)', friend) }
scope :invitation_requests, ->(user) { where('friend_id = (?)', user).where('status = (?)', false) }
Copy link

@mosaaleb mosaaleb Oct 1, 2019

Choose a reason for hiding this comment

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

  • [OPTIONAL] There is some sort of logic duplication in these scopes. For example:
  • invitation_requests scope can be achieved with user.passive_friendships
  • invitation_sent scope can also be achieved with user.active_friendships

I suggest to use association because defining associations can give your number of other helper methods like passive_friendship_ids which will return an array of friendships records ids associated with a particular user.

Also passive_friendships return value acts like an array provided also with the helper methods (though it is not actually an array), so you can do something like user.passive_friendships << [friendship_object].

Copy link
Owner Author

Choose a reason for hiding this comment

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

how could I use associations for that purpose?

Copy link

Choose a reason for hiding this comment

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

@samgaco Sorry, what do you mean?

<%= "By: " + User.find(post.user_id).name %> <br>
<div class="post-field">
<%= post.content %> <br>
<%= "By: " + User.find(post.user_id).name %> <br>
Copy link

@mosaaleb mosaaleb Oct 1, 2019

Choose a reason for hiding this comment

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

  • Views should not perform any application logic. We can replace User.find(post.user_id).name with just post.user.name

Copy link
Owner Author

Choose a reason for hiding this comment

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

solved


<div class="field-user">

<% if (current_user.friendships.where('user_id = :id OR friend_id = :id', id: user.id).where("status = (?)", true).exists? )%>
Copy link

@mosaaleb mosaaleb Oct 1, 2019

Choose a reason for hiding this comment

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

  • This view file is bloated with functionality that doesn't belong to it.
    (current_user.friendships.where('user_id = :id OR friend_id = :id', id: user.id).where("status = (?)", true).exists? ) belongs to model. So we can create accepted_friendship scope whose status is true and make it available in the controller so it will be availble in the views. Views aren't supposed to hit the database to get data, it just displays the data provided by its corresponding controller.

Copy link
Owner Author

@samgaco samgaco left a comment

Choose a reason for hiding this comment

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

Well done @samgaco

  • sender sent a friend request to receiver. reciever can't send a friend request to sender.

friendship

We can achieve this by adding interchangeable validation to check if the [user_id, friend_id] or [friend_id, user_id] exist in the friendship table. please take a look at this example

  • [OPTIONAL] Previous step adds model validations to friendships uniqueness. I would suggest to add interchangeable DB validations. You need to know first why DB validations are important why.
    My previous code partner wrote an article on how to achieve this.
  • Please see my comments below.
Please handle these requirements and submit a new code review.
Happy Coding

Changes made as suggested

Copy link

@Oghenebrume50 Oghenebrume50 left a comment

Choose a reason for hiding this comment

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

Hi @samgaco good work 👏 this is approved, please move to the next milestone

@samgaco samgaco merged commit 1300ca2 into development Oct 3, 2019
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.

3 participants