Skip to content
Snippets Groups Projects

WIP: Show Releases Link in Sidebar for Guests

2 unresolved threads

What does this MR do?

Show the Releases link for guests viewing private repositories.

image

Does this MR meet the acceptance criteria?

Closes #56469 (closed)

Edited by Douwe Maan

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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 of get_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 and tab_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 as Gitlab.config.registry.enabled so let's put it outside from tab_ability_map". But to me, this kind of extention logic should be still a responsibility of tab_ability_map and its iterator. This sounds like we're piling up ~"technical debt" again and again.

  • Andrew Fontaine created #56820 (moved) to continue this discussion

    created #56820 (moved) to continue this discussion

  • @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.

  • Please register or sign in to reply
  • Shinya Maeda
  • @afontaine Thank you for diving into this issue! I left a few notes. Nice Fix!

  • @dosuken123 I'm not 100% sure how to fix up your notes properly, want to take another look and make sure my proposed solution looks ok?

  • Andrew Fontaine assigned to @dosuken123

    assigned to @dosuken123

  • @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.

  • Sure, I can fix up the test and create a follow-up issue for the ~"technical debt".

    We could reduce things to a lazy iterator and some lambdas, but I don't know if that would wreck performance somehow, and this code does run on a lot of pages...

  • added 1 commit

    • 8f5b4731 - Add Helper to Fetch Permissions in Test

    Compare with previous version

  • assigned to @DouweM

  • @DouweM can you give this a final look over?

  • added 1 commit

    • c8493b50 - Add Helper to Fetch Permissions in Test

    Compare with previous version

  • Andrew Fontaine resolved all discussions

    resolved all discussions

  • mentioned in issue #56820 (moved)

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 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)
    • I don't love this, because we're effectively redefining can? with its original implementation, reverting the earlier allow(helper).to receive(:can?) { true }. Would it be an option to use allow(helper).to receive(:can?).and_call_original instead?

    • Do we need this call at all? It doesn't look like the allow(helper).to receive(:can?) { true } above still applies to us.

    • @DouweM Without the stub_delegation_for_can call, it seems can? doesn't exist on the ProjectsHelper. I imagine it's why can? is mocked in the other tests to begin with. Where does can? 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 the helper_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.

    • Please register or sign in to reply
  • assigned to @afontaine

  • added 1 commit

    • ff63a50d - Move Authorization Checks to Their Own Function

    Compare with previous version

  • @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.

  • Douwe Maan marked as a Work In Progress

    marked as a Work In Progress

  • closed

  • Please register or sign in to reply
    Loading