WIP: Show Releases Link in Sidebar for Guests
What does this MR do?
Show the Releases link for guests viewing private repositories.
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
Documentation reviewed by technical writer or follow-up review issue created -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer
Closes #56469 (closed)
Merge request reports
Activity
changed milestone to %11.8
assigned to @afontaine
assigned to @shampton
@shampton Want to review this?
@afontaine This looks fine to me, but I'm not extremely experienced when it comes to Ruby. It might be better to get a backend engineer to approve this. Maybe @dosuken123 or straight to a maintainer like @rspeicher.
assigned to @afontaine
321 322 nav_tabs.flatten 323 end 324 325 def get_project_nav_tab_abilities(project, current_user, nav_tabs) 326 nav_tabs << :releases if !project.empty_repo? && can?(current_user, :read_release, project) 327 320 328 tab_ability_map.each do |tab, ability| 321 329 if can?(current_user, ability, project) 322 330 nav_tabs << tab 323 331 end 324 332 end 325 333 326 nav_tabs.flatten 334 nav_tabs 327 335 end Isn't this essentially same with
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 2a24aa2a382..535ee00547c 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -296,7 +296,11 @@ module ProjectsHelper nav_tabs = [:home] if !project.empty_repo? && can?(current_user, :download_code, project) - nav_tabs << [:files, :commits, :network, :graphs, :forks, :releases] + nav_tabs << [:files, :commits, :network, :graphs, :forks] + end + + if !project.empty_repo? && can?(current_user, :read_release, project) + nav_tabs << :releases end if project.repo_exists? && can?(current_user, :read_merge_request, project)
?
What's the purpose of
get_project_nav_tab_abilities
method?rubocop
complained about the cyclomatic and complexity ofget_project_nav_tabs
in 604b6db0, so I extracted the:read_
-related checks into a new method. Would it be better to add an exception or avoid it some other way?Maybe then
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 2a24aa2a382..d842c9e2b6b 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -294,10 +294,8 @@ module ProjectsHelper def get_project_nav_tabs(project, current_user) nav_tabs = [:home] - - if !project.empty_repo? && can?(current_user, :download_code, project) - nav_tabs << [:files, :commits, :network, :graphs, :forks, :releases] - end + nav_tabs << [:files, :commits, :network, :graphs, :forks, :releases] if can_download_code?(current_user, project) + nav_tabs << :releases if can_read_release?(current_user, project) if project.repo_exists? && can?(current_user, :read_merge_request, project) nav_tabs << :merge_requests @@ -328,6 +326,14 @@ module ProjectsHelper nav_tabs.flatten end + def can_download_code?(user, project) + !project.empty_repo? && can?(current_user, :download_code, project) + end + + def can_read_release?(user, project) + !project.empty_repo? && can?(user, :read_releases, project) + end + def tab_ability_map { environments: :read_environment,
?
The problem to me is that
get_project_nav_tab_abilities
does not do exactly the thing the method name tells. Maintainers might wonder what the purpose is.Alternatively, you can solve this issue by creating a follow-up issue. It seems to me
get_project_nav_tabs
andtab_ability_map
should be in one organized class. Currently,get_project_nav_tabs
just tells "Hey,tab_ability_map.each do |tab, ability|
cannot check additional condition such asGitlab.config.registry.enabled
so let's put it outside fromtab_ability_map
". But to me, this kind of extention logic should be still a responsibility oftab_ability_map
and its iterator. This sounds like we're piling up ~"technical debt" again and again.created #56820 (moved) to continue this discussion
@afontaine I would rather not wait to fix this in a follow-up issue; see https://gitlab.com/gitlab-org/gitlab-ce/blob/0c13bffbdc121513ef8d586b0e912cfcd7ee3169/doc/development/contributing/issue_workflow.md#technical-debt-in-follow-up-issues. Did you try @dosuken123's proposal in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24540#note_134375157?
@DouweM The patch works, but I'm not sure it really tackles the debt that @dosuken123 pointed out it the second half. I took a stab at how I'd go about simplifying/unifying. Thoughts on the approach? How could I clean it up?
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index a8063aa112f..461547c7626 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -291,63 +291,39 @@ module ProjectsHelper private def get_project_nav_tabs(project, current_user) - nav_tabs = [:home] - - nav_tabs << [:files, :commits, :network, :graphs, :forks] if can_download_code?(project, current_user) - - nav_tabs << :releases if can_read_release?(project, current_user) - - if project.repo_exists? && can?(current_user, :read_merge_request, project) - nav_tabs << :merge_requests - end - - if Gitlab.config.registry.enabled && can?(current_user, :read_container_image, project) - nav_tabs << :container_registry - end - - if project.builds_enabled? && can?(current_user, :read_pipeline, project) - nav_tabs << :pipelines - end - - if can?(current_user, :read_environment, project) || can?(current_user, :read_cluster, project) - nav_tabs << :operations - end - - if project.external_issue_tracker - nav_tabs << :external_issue_tracker - end - - tab_ability_map.each do |tab, ability| - if can?(current_user, ability, project) - nav_tabs << tab - end - end - - nav_tabs.flatten - end - - def can_download_code?(project, current_user) - !project.empty_repo? && can?(current_user, :download_code, project) - end - - def can_read_release?(project, current_user) - !project.empty_repo? && can?(current_user, :read_release, project) + tab_ability_map(current_user, project).lazy + .select { |_tab, ability| ability.call() } + .map { |tab, _ability| tab } end - def tab_ability_map + def tab_ability_map(current_user, project) + can_do = lambda { |x| lambda { can?(current_user, x, project) } } + can_download_code = lambda { !project.empty_repo? && can_do.call(:download_code).call() } { - environments: :read_environment, - milestones: :read_milestone, - snippets: :read_project_snippet, - settings: :admin_project, - builds: :read_build, - clusters: :read_cluster, - serverless: :read_cluster, - error_tracking: :read_sentry_issue, - labels: :read_label, - issues: :read_issue, - project_members: :read_project_member, - wiki: :read_wiki + home: lambda { true }, + files: can_download_code, + commits: can_download_code, + network: can_download_code, + graphs: can_download_code, + forks: can_download_code, + releases: lambda { !project.empty_repo? && can_do.call(:read_release).call() }, + merge_requests: lambda { project.repo_exists? && can_do.call(:read_merge_requests).call() }, + container_registry: lambda { Gitlab.config.registry.enabled && can_do.call(:read_container_image).call() }, + pipelines: lambda { project.builds_enabled? && can_do.call(:read_pipeline).call() }, + operations: lambda { can_do.call(:read_environment).call() || can_do.call(:read_cluster).call() }, + external_issue_tracker: lambda { project.external_issue_tracker }, + environments: can_do.call(:read_environment), + milestones: can_do.call(:read_milestone), + snippets: can_do.call(:read_project_snippet), + settings: can_do.call(:admin_project), + builds: can_do.call(:read_build), + clusters: can_do.call(:read_cluster), + serverless: can_do.call(:read_cluster), + error_tracking: can_do.call(:read_sentry_issue), + labels: can_do.call(:read_label), + issues: can_do.call(:read_issue), + project_members: can_do.call(:read_project_member), + wiki: can_do.call(:read_wiki) } end
Edited by Andrew Fontaine@afontaine Interesting proposal! It looks a little odd to a Rubyist right now, but that, I think we can resolve to the follow-up ~"technical debt" issue :) What I'd like to resolve in this MR is the first part of @dosuken123's concern:
The problem to me is that
get_project_nav_tab_abilities
does not do exactly the thing the method name tells. Maintainers might wonder what the purpose is.I feel like https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24540#note_134375157 is already better than what we have in this MR, even if it doesn't go "all the way" yet.
- Resolved by Andrew Fontaine
@afontaine Thank you for diving into this issue! I left a few notes. Nice Fix!
assigned to @afontaine
@afontaine Thank you. I left a couple of suggestions. Please feel free to resolve these comments as follow-up issue. Else, LGTM and please pass to a maintainer.
assigned to @afontaine
assigned to @DouweM
@DouweM can you give this a final look over?
mentioned in issue #56820 (moved)
353 351 before do 354 352 allow(project).to receive(:builds_enabled?).and_return(false) 353 allow(helper).to receive(:can?) { true } 355 354 end 356 355 357 356 it "do not include pipelines tab" do 358 357 is_expected.not_to include(:pipelines) 359 358 end 360 359 end 360 361 context 'when project is private' do 362 let(:project) { create(:project, :private, :repository) } 363 364 context 'when user is guest' do 365 before do 366 stub_delegation_for_can(helper) @DouweM Without the
stub_delegation_for_can
call, it seemscan?
doesn't exist on theProjectsHelper
. I imagine it's whycan?
is mocked in the other tests to begin with. Where doescan?
come from?Failures: 1) ProjectsHelper#get_project_nav_tabs when project is private when user is guest gets release tab Failure/Error: if !project.empty_repo? && can?(current_user, :download_code, project) NoMethodError: undefined method `can?' for #<#<Class:0x00007fb9941b0e40>:0x00007fb9a04665e8> # ./app/helpers/projects_helper.rb:296:in `get_project_nav_tabs' # ./spec/helpers/projects_helper_spec.rb:336:in `block (3 levels) in <top (required)>' # ./spec/helpers/projects_helper_spec.rb:371:in `block (5 levels) in <top (required)>' Finished in 19.25 seconds (files took 8.69 seconds to load) 108 examples, 1 failure
Edited by Andrew Fontaine@afontaine Ah, interesting! I didn't realize it wouldn't normally be available. It's defined in
ApplicationController
and from there made accessible to views and helpers using thehelper_method
method.Then I think we can leave this, although I'm curious how we've solved this in the past and why this is the first time we need to solve it this way.
assigned to @afontaine
added 1 commit
- ff63a50d - Move Authorization Checks to Their Own Function
@DouweM I've fixed up the MR according to your review comments.
assigned to @DouweM
mentioned in issue #56402 (closed)
@afontaine Thank you! I'm going to mark this WIP and hold off on reviewing/merging until we get resolution on https://gitlab.com/gitlab-org/gitlab-ce/issues/56402#note_135541677, though.
I'm closing this in light of https://gitlab.com/gitlab-org/gitlab-ce/issues/56469#note_135605822!
added devopsrelease [DEPRECATED] label