Skip to content

Commit

Permalink
Fix reviewable_by for by_package
Browse files Browse the repository at this point in the history
by_package always implies by_project as well - just the package
name matching isn't good enough. See obs request 669918 where
it was impossible to approve the sle-workerarounds reviews
  • Loading branch information
coolo committed Feb 4, 2019
1 parent 9faf738 commit a16c7cf
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 6 deletions.
12 changes: 8 additions & 4 deletions src/api/app/models/review.rb
Expand Up @@ -225,10 +225,14 @@ def create_notification(params = {})
end

def reviewable_by?(opts)
by_user && by_user == opts[:by_user] ||
by_group && by_group == opts[:by_group] ||
by_project && by_project == opts[:by_project] ||
by_package && by_package == opts[:by_package]
return by_user == opts[:by_user] if by_user
return by_group == opts[:by_group] if by_group
reviewable_by = by_project == opts[:by_project]
if by_package
reviewable_by && by_package == opts[:by_package]
else
reviewable_by
end
end

def change_state(new_state, comment)
Expand Down
6 changes: 4 additions & 2 deletions src/api/spec/models/review_spec.rb
Expand Up @@ -407,6 +407,7 @@
let(:other_group) { create(:group, title: 'my_group') }
let(:other_project) { create(:project_with_package, name: 'doc:things', package_name: 'less') }
let(:other_package) { other_project.packages.first }
let(:other_package_with_same_name) { create(:package, name: package.name) }

let(:review_by_user) { create(:review, by_user: user.login) }
let(:review_by_group) { create(:review, by_group: group.title) }
Expand All @@ -417,14 +418,15 @@
expect(review_by_user.reviewable_by?(by_user: user.login)).to be(true)
expect(review_by_group.reviewable_by?(by_group: group.title)).to be(true)
expect(review_by_project.reviewable_by?(by_project: project.name)).to be(true)
expect(review_by_package.reviewable_by?(by_package: package.name)).to be(true)
expect(review_by_package.reviewable_by?(by_package: package.name, by_project: package.project.name)).to be(true)
end

it 'returns false if review configuration does not match provided hash' do
expect(review_by_user.reviewable_by?(by_user: other_user.login)).to be_falsy
expect(review_by_group.reviewable_by?(by_group: other_group.title)).to be_falsy
expect(review_by_project.reviewable_by?(by_project: other_project.name)).to be_falsy
expect(review_by_package.reviewable_by?(by_package: other_package.name)).to be_falsy
expect(review_by_package.reviewable_by?(by_package: other_package.name, by_project: other_package.project.name)).to be_falsy
expect(review_by_package.reviewable_by?(by_package: other_package_with_same_name.name, by_project: other_package_with_same_name.project.name)).to be_falsy
end
end

Expand Down

0 comments on commit a16c7cf

Please sign in to comment.