Skip to content
Snippets Groups Projects

Resolve "Destroying a project causes post_decline_request to be executed"

Merged Resolve "Destroying a project causes post_decline_request to be executed"
18755-fix-destroy-project-causes-post_decline_request-to-be-executed into master
Merged Rémy Coutable requested to merge 18755-fix-destroy-project-causes-post_decline_request-to-be-executed into master

What does this MR do?

Ensure we don't send "access request declined" to access requesters when a project is deleted.

Are there points in the code the reviewer needs to double check?

I've created a service to decouple the notification sending from the AR model.

Why was this MR needed?

Because there was an issue.

What are the relevant issue numbers?

Fixes #18755 (closed), #18750 (closed).

Does this MR meet the acceptance criteria?

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
  • Douwe Maan
  • Douwe Maan Reassigned to @rymai

    Reassigned to @rymai

  • Rémy Coutable Added 1 commit:

    Added 1 commit:

    • ec7d2328 - Raise a new Gitlab::Access::AccessDeniedError when permission is not enough to destroy a member
  • Rémy Coutable Reassigned to @DouweM

    Reassigned to @DouweM

  • Douwe Maan
    Douwe Maan @DouweM ·
    Contributor

    @rymai Do we still have special error message behavior for the cannot_delete? case of being last owner? I think we should. AccessDenied and error 403 only makes sense if the user tries to do something they're not allowed to.

    Edited by Douwe Maan
  • Douwe Maan
  • Douwe Maan Reassigned to @rymai

    Reassigned to @rymai

  • Rémy Coutable Added 473 commits:

    Added 473 commits:

    • ec7d2328...00906b5b - 466 commits from branch master
    • 4652489f - New Members::DestroyService
    • 6c5b2377 - Use the new Members::DestroyService in group/project member controllers
    • 724f986f - Redirect to the member's source on request withdrawal
    • a08a26ac - Don't send the "access declined" email on access request withdrawal
    • 654565c9 - Raise a new Gitlab::Access::AccessDeniedError when permission is not enough to destroy a member
    • bceee987 - Show 'Leave project' only if member can actually leave the project
    • bf05ca88 - Add 'Leave Group' link
    Toggle commit list
  • Rémy Coutable
    Rémy Coutable @rymai started a thread on the diff
    • app/views/layouts/nav/_project_settings.html.haml
    3 3 = link_to namespace_project_project_members_path(@project.namespace, @project), title: 'Members', class: 'team-tab tab' do
    4 4 %span
    5 5 Members
    6 if access && can_edit
    6 if can_edit
    • Rémy Coutable
      Rémy Coutable @rymai ·
      Author Maintainer

      There was no point checking for access here since can_edit is stricter than access... That being said, I'm not sure restricting this menu to users that can edit the project is super smart... Anyways, I didn't change that, just removed a useless condition.

  • Rémy Coutable
    Rémy Coutable @rymai started a thread on the diff
    • app/views/layouts/nav/_group_settings.html.haml
    3 - member = @group.members.non_request.find_by(user_id: current_user.id)
    4 - can_leave = member && can?(current_user, :destroy_group_member, member)
    5
    6 .controls
    7 .dropdown.group-settings-dropdown
    8 %a.dropdown-new.btn.btn-default#group-settings-button{href: '#', 'data-toggle' => 'dropdown'}
    9 = icon('cog')
    10 = icon('caret-down')
    11 %ul.dropdown-menu.dropdown-menu-align-right
    12 = nav_link(path: 'groups#projects') do
    13 = link_to 'Projects', projects_group_path(@group), title: 'Projects'
    14 %li.divider
    15 - if can_edit
    16 %li
    17 = link_to 'Edit Group', edit_group_path(@group)
    18 - if can_leave
    • Rémy Coutable
      Rémy Coutable @rymai ·
      Author Maintainer

      @DouweM I re-added the "Leave Group" link (I think I removed it inadvertently in the request access MR), and displays it only when the current user can actually leave the group (same for project in app/views/layouts/nav/_project.html.haml).

      I think this makes more sense than displaying the link and then showing a "You can not leave this project" alert once it's clicked... Also, we're already using this logic in the members list page.

      error 403 only makes sense if the user tries to do something they're not allowed to.

      Now, this is the case!

  • Rémy Coutable Added 1 commit:

    Added 1 commit:

    • 909a0ff3 - Fix and remove duplicate specs
  • Rémy Coutable Reassigned to @DouweM

    Reassigned to @DouweM

  • Douwe Maan Status changed to merged

    Status changed to merged

  • Douwe Maan mentioned in commit c11006ac

    mentioned in commit c11006ac

  • Rémy Coutable
    Rémy Coutable @rymai ·
    Author Maintainer

    Picked into 8-9-stable, will go into 8.9.1.

  • Rémy Coutable Removed ~149423 label

    Removed ~149423 label

  • Rémy Coutable mentioned in issue #18970 (closed)

    mentioned in issue #18970 (closed)

  • Rémy Coutable mentioned in issue #18790 (closed)

    mentioned in issue #18790 (closed)

  • Douwe Maan mentioned in commit 9af7d71b

    mentioned in commit 9af7d71b

  • Douwe Maan Mentioned in commit jmcgeheeiv/gitlab-ce@c11006ac

    Mentioned in commit jmcgeheeiv/gitlab-ce@c11006ac

  • Douwe Maan Mentioned in commit xhang/gitlab-ce@9af7d71b

    Mentioned in commit xhang/gitlab-ce@9af7d71b

    • Please register or sign in to reply