#8029 Submitter should be able to reject merge request
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
https://forge-allura.apache.org/u/pranav/allura/ci/d63bde6de572a4a888012f6bae4e4c45750786cf/
I have added a button which would reject the merge reject. It would only be visible to the submitter of the request.
Last edit: Pranav Sharma 2016-01-11
The new
rejectmethod has several lines that are the same as thesavemethod. It'd be better to share the same method, so there's not repeated code. Can you change thesavemethod to incorporate the additional permission checking, and have the form submit there?The added
get_creator()method looks the same as the.creatorproperty defined right above it. Can you use.creatorinstead?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
Improved.
https://forge-allura.apache.org/u/pranav/allura/ci/69d8dc5876cb1bae79bd06c2b4417b24663a38dd/
The
@LazyPropertydecorator should be kept on thedef creatormethod/property. Removing it will change functionality and likely break other things elsewhere that use it. What it does is very much like the standard@propertydecorator: 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_statusis 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.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.
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?
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_numLast edit: Pranav Sharma 2016-01-16
https://forge-allura.apache.org/p/allura/git/merge-requests/89/