#8029 Submitter should be able to reject merge request

v1.4.0
closed
General
nobody
2016-03-14
2015-12-08
No

The submitter of a merge request should be able to change its status to "rejected" (or possibly delete it, but I think that'd be going too far). [#5993] for updating an existing request will help too, but there's still a good case for rejecting your own MR I think.

This has been requested by several SF users: https://sourceforge.net/p/forge/feature-requests/254/

Related

Git: 01846013
Git: 5e15a766
Git: 915eaf88
Git: bebfa87b
Git: ec763858
Tickets: #5993

Discussion

  • Pranav Sharma

    Pranav Sharma - 2016-01-11
    • status: open --> in-progress
    • assigned_to: Pranav Sharma
     
  • Dave Brondsema

    Dave Brondsema - 2016-01-11
    • status: in-progress --> review
     
  • Dave Brondsema

    Dave Brondsema - 2016-01-11
    • status: review --> in-progress
     
  • Dave Brondsema

    Dave Brondsema - 2016-01-11

    The new reject method has several lines that are the same as the save method. It'd be better to share the same method, so there's not repeated code. Can you change the save method to incorporate the additional permission checking, and have the form submit there?

    The added get_creator() method looks the same as the .creator property defined right above it. Can you use .creator instead?

    The reject button is right up against the sidebar. Can you put a <div class="grid-19"> around it like the Merge button has? Then it should have some spacing.

    Thanks

     
    • Pranav Sharma

      Pranav Sharma - 2016-01-12
       
      • Dave Brondsema

        Dave Brondsema - 2016-01-13

        The @LazyProperty decorator should be kept on the def creator method/property. Removing it will change functionality and likely break other things elsewhere that use it. What it does is very much like the standard @property decorator: it becomes an attribute that you access without (). And the "Lazy" part of this decorator is custom, it caches the result so that if you reference it again, it doesn't re-run the code, just uses the saved result again. Several tests do fail, potentially because of this.

        The form doesn't actually work right now. It doesn't specify the status field when it submits.

        What if a non-creator tries to reject a merge request? Doesn't look like the logic supports that any more, but it needs to.

        There still is duplicated code, although its in the same method now. Can you get all the permission checking done first, and then have just one section of code with the render/add_post/status lines?

        Adding a test for this new situation would be good. test_merge_request_update_status is an example to look at. You'd have to change it to use a non-admin to create the merge request and then reject it.

         
        • Pranav Sharma

          Pranav Sharma - 2016-01-14

          Thanks Dave. I now understood the use of LazyDecorator.

          For now, only the current admins of the project are allowed to change the status of a merge request. I am trying to give the rejection modification to the submitter. But are you asking it to be given to more than these people? If so, a person might just spam rejecting useful merge request of other submitters.

           
          • Dave Brondsema

            Dave Brondsema - 2016-01-14

            Just admins and the submitter is correct. I mean what if an admin (who isn't the creator) wanted to change the status to rejected?

             
            • Pranav Sharma

              Pranav Sharma - 2016-01-14

              I have not tested that part. The list which has status changes, below discuss panel is visible only to the creator and not the admins?

              Update 1: I have now tested the situation. The admins of a project can modify the status of merge requests from the list below the discuss panel, just above the comment section.

              Update 2 :
              I have problm uderstanding below section of code. While running on localhost, '/p/myproject/code' takes me to the repo, but 'request_merge' gives an error.

              def _request_merge(self, **kw):
                  r = self.app.get('/p/test2/code/request_merge', **kw)
                  r = self._follow(r, **kw)
                  form = self._find_request_merge_form(r)
                  r = form.submit()
                  r = self._follow(r, **kw)
                  mr_num = r.request.url.split('/')[-2]
                  assert mr_num.isdigit(), mr_num
                  return r, mr_num
              
               

              Last edit: Pranav Sharma 2016-01-16
  • Dave Brondsema

    Dave Brondsema - 2016-03-14
    • status: in-progress --> closed
     
  • Dave Brondsema

    Dave Brondsema - 2016-04-11
    • Milestone: unreleased --> v1.4.0
     

Log in to post a comment.