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

Refactoring user mode (involved_reviews, involved_projects, involved_packages) #3105

Merged
merged 5 commits into from May 31, 2017

Conversation

Projects
None yet
3 participants
@ChrisBr
Copy link
Member

ChrisBr commented May 11, 2017

No description provided.

@ChrisBr ChrisBr force-pushed the ChrisBr:involved_reviews branch 3 times, most recently from 8495a0b to 0a7c48e May 11, 2017

@@ -34,6 +34,12 @@ class User < ApplicationRecord
has_many :relationships, inverse_of: :user, dependent: :destroy
has_many :group_relationships, through: :groups, source: :relationships

has_many :direct_maintainer_relationships, -> { where(role_id: Role.hashed['maintainer'].id) }, class_name: 'Relationship'

This comment has been minimized.

@hennevogel

hennevogel May 15, 2017

Member

direct is to distinguish this relationship from the ones over the group relationship? So there are direct relations and group relations?

This comment has been minimized.

@ChrisBr

ChrisBr May 19, 2017

Member

Yes, exactly!

This comment has been minimized.

@hennevogel

hennevogel May 19, 2017

Member

I would call them user_ so it's clear what the difference is...

This comment has been minimized.

@ChrisBr

ChrisBr May 19, 2017

Member

Ok, fine for me!

@ChrisBr ChrisBr force-pushed the ChrisBr:involved_reviews branch 2 times, most recently from e69b244 to c1165ff May 19, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 29, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 29, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 29, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 29, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 29, 2017

@ChrisBr ChrisBr force-pushed the ChrisBr:involved_reviews branch from c1165ff to f7a8eab May 29, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 30, 2017

@hennevogel please have a look again

packages += Relationship.packages.where(role_id: role.id).joins(:groups_users).where(groups_users: { user_id: id }).pluck(:package_id)

Package.where(id: packages).where('project_id not in (?)', projects)
Package.where(id: involved_packages_ids)

This comment has been minimized.

@evanrolfe

evanrolfe May 30, 2017

A 12 line method is now 1 line.. Very cool 😎

BsRequest.where(reviews: { reviewable_id: involved_projects_ids, reviewable_type: 'Project' }).or(
BsRequest.where(reviews: { reviewable_id: involved_packages_ids, reviewable_type: 'Package' }).or(
BsRequest.where(reviews: { reviewable_id: groups.pluck(:id), reviewable_type: 'Group' })
)

This comment has been minimized.

@evanrolfe

evanrolfe May 30, 2017

I'm thinking, maybe we can use scopes to make this nicer? i.e.

bs_request.rb

  scope :for_users, ->(user_ids) { where(reviewable_id: user_ids, reviewable_type: 'User') }
  scope :for_projects, ->(project_ids) { where(reviewable_id: project_ids, reviewable_type: 'Project') }
  scope :for_packages, ->(package_ids) { where(reviewable_id: package_ids, reviewable_type: 'Package') }
  scope :for_groups, ->(group_ids) { where(reviewable_id: group_ids, reviewable_type: 'Group') }

user.rb

  def involved_reviews(search = nil)
    BsRequest.for_users([id]).or(
      BsRequest.for_projects(involved_projects_ids).or(
        BsRequest.for_packages(involved_packages_ids).or(
          BsRequest.for_groups(groups.pluck(:id))
        )
      )
    )
  end

This comment has been minimized.

@ChrisBr

ChrisBr May 30, 2017

Member

Okay, fixed!

@@ -903,6 +900,16 @@ def update_notifications(params)

private

def involved_packages_ids
user_maintained_packages.where.not(project_id: involved_projects_ids).pluck(:id) +
group_maintained_packages.where.not(project_id: involved_projects_ids).pluck(:id)

This comment has been minimized.

@evanrolfe

evanrolfe May 30, 2017

Is there a way that we could keep this as an ActiveRecord::Relation object? Because currently we loads the packages, then get and id of arrays for them, and then load the packages again from those ids in involved_packages method. I'm not sure if its possible but maybe something like this could work?

user_maintained_packages.group_maintained_packages

Since both those scopes should just be where clauses I think you can chain them together like that?

This comment has been minimized.

@ChrisBr

ChrisBr May 30, 2017

Member

They're both relationships and they're structurally incompatible, so e.g.

user_maintained_packages.or(group_maintained_packages)
does not work:
Relation passed to #or must be structurally compatible. Incompatible values: [:joins, :references]
(They joins are different: User -> Relationship -> Package vs. User -> Groups -> Relationships -> Package)

I also don't really like this ... 😢

Same is also the case for involved_projects

This comment has been minimized.

@ChrisBr

ChrisBr May 30, 2017

Member

After thinking about it:

Package.joins(:relationships).where(relationships: { user_id: id }).or(
  Package.joins(:relationships).where(relationships: { group_id: groups.pluck(:id) })
)

What do you think about it?

@ChrisBr ChrisBr force-pushed the ChrisBr:involved_reviews branch from f7a8eab to 35016f1 May 30, 2017

@ChrisBr

This comment has been minimized.

Copy link
Member

ChrisBr commented May 30, 2017

@evanrolfe please have a look again!

@ChrisBr ChrisBr changed the title Refactoring BsRequest.collection in user model Refactoring user mode (involved_reviews, involved_projects, involved_packages) May 30, 2017

@ChrisBr ChrisBr force-pushed the ChrisBr:involved_reviews branch from 35016f1 to ab5a443 May 30, 2017

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 30, 2017

Codecov Report

Merging #3105 into master will increase coverage by <.01%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3105      +/-   ##
==========================================
+ Coverage   88.93%   88.93%   +<.01%     
==========================================
  Files         263      263              
  Lines       17587    17588       +1     
==========================================
+ Hits        15641    15642       +1     
  Misses       1946     1946
Flag Coverage Δ
#api 83.67% <82.35%> (-0.02%) ⬇️
#rspec 65.96% <88.23%> (ø) ⬆️
#webui 64.22% <94.11%> (-0.26%) ⬇️
Package.where(id: packages).where('project_id not in (?)', projects)
Package.for_user(id).or(
Package.for_group(groups.pluck(:id))
).where.not(project_id: involved_projects.pluck(:id))

This comment has been minimized.

@evanrolfe

evanrolfe May 30, 2017

Perfect. Thanks

@evanrolfe

This comment has been minimized.

Copy link

evanrolfe commented May 30, 2017

LGTM (when circleCI passes)

@ChrisBr ChrisBr force-pushed the ChrisBr:involved_reviews branch from ab5a443 to 8acb626 May 30, 2017

ChrisBr added some commits May 10, 2017

[webui][api] Refactor involved_packages
and use user associations instead.
[webui][api] Refactor involved_reviews
to get rid of BsRequest.collection and use ActiveRecord instead.
[ci] Fix reviews fixtures
and add polymorphic association.
[webui] Introduce scopes for User#involved_reviews
to make code more clear and readable.
[webui] Refactor User#involved_projects
- Introduced scopes
- Removed helper method involved_projects_ids to avoid database roundtrips

@ChrisBr ChrisBr force-pushed the ChrisBr:involved_reviews branch from 8acb626 to 7e344a7 May 31, 2017

@ChrisBr ChrisBr merged commit b4d8ca6 into openSUSE:master May 31, 2017

4 checks passed

Hakiri No security warnings were found.
Details
codecov/patch 94.11% of diff hit (target 88.93%)
Details
codecov/project 88.93% (+<.01%) compared to c929ed9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ChrisBr ChrisBr deleted the ChrisBr:involved_reviews branch Jul 18, 2017

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