My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 41: Cannot abandon a change
  Back to list
Status:  Released
Owner:  code-rev...@gtempaccount.com
Closed:  Oct 2012


Sign in to add a comment
 
Reported by code-rev...@gtempaccount.com, Sep 24, 2009
Reported by Shawn Pearce <sop@google.com> on Tue Jan 27 09:06:28 PST 2009
Source: JIRA GERRIT-41
Affected Version: 2.0

Its impossible to mark a change as abandoned.
Sep 24, 2009
#1 code-rev...@gtempaccount.com
Comment by Brad Larson <bklarson@gmail.com> on Sat Feb 14 11:43:26 PST 2009

How complicated is this feature?

If it is mostly UI work and some RPC plumbing, I'd be glad to look into it.
However, if it will involve more lower-level work with git or gwtorm, I should
probably hold off for now.

Will a new permission group be required to abandon a change, or is the submit
permission sufficient?  What code review/verified (dis)approvals need to be
applied before a change can be abandoned?
Sep 24, 2009
#2 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Mon Feb 16 08:16:30 PST 2009

I think its all UI and RPC, there shouldn't be any Git or ORM work needed here.

IMHO, an open change can be marked abandoned at any time by:

- The change owner
- The current patch set uploader
- The project owner

Marking it abandoned should just be a matter of updating the Change with
setStatus(Change.Status.ABANDONED) and writing it back to the database.  We
probably also should send out an email to all current reviewers, and perhaps
permit an optional "cover letter" message to go into a ChangeMessage entity so
the person marking it abandoned has a chance to attach a short message *why*.
This might just be a link to another change record that has superseded this
one.
Sep 24, 2009
#3 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Wed Feb 18 09:10:55 PST 2009

Assigned to Brad Larson.
Sep 24, 2009
#4 code-rev...@gtempaccount.com
Comment by Brad Larson <bklarson@gmail.com> on Tue Feb 24 13:44:12 PST 2009

Should a user need a permission in order to abandon a change?
Currently in PatchSetPanel.java line 280, this is required:

 for (final ApprovalType at : Common.getGerritConfig().getActionTypes())=
 {
     final ApprovalCategoryValue max =3D at.getMax();
     if (max =3D=3D null || max.getValue() <=3D 0) {
       // No positive assertion, don't draw a button.
       continue;
     }
Sep 24, 2009
#5 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Tue Feb 24 13:48:14 PST 2009

Heh.

That block isn't about a per-user permission, its about whether or not there
is a positive value in the approval_category_values table for this action.

We could just insert that value at the same time that we define the
approval_categories row for the abandon permission.  Just like submit, the
table only needs a +1 value row.
Sep 24, 2009
#6 code-rev...@gtempaccount.com
Comment by Brad Larson <bklarson@gmail.com> on Tue Feb 24 13:53:35 PST 2009

Ah I see... adding a value to the approval_category_values isn't a problem.
Will this add an entry to the group permissions as well?  That's what I was
hoping to avoid.  I'll play around with things and see what happens.
Sep 24, 2009
#7 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Tue Feb 24 13:55:03 PST 2009

Yes.  It will wind up being something that can be granted to groups in a
project's Access panel.  :-|
Sep 24, 2009
#8 code-rev...@gtempaccount.com
Comment by Brad Larson <bklarson@gmail.com> on Tue Feb 24 20:35:53 PST 2009

I have a work around... not sure if you'll like it...

I can add a method to CategoryFunction:
  public boolean needsPermissions() {
    return true;
  }

Then modify the code in PatchSetPanel like so:
      if (!allowed.contains(at.getCategory().getId())
          && at.getCategory().getFunction().needsPermissions()) {
        // User isn't permitted to invoke this.
        continue;
      }

Currently just AbandonFunction overrides needsPermissions to return false.

With something like this, we don't have to add the 'Abandon' permission to
every user.  It isn't ideal, but it might be the best option for how the code
is currently laid out.


Another issue which will require a little more investigation - I'd like to pop
up a dialog box when the user clicks 'abandon' so they can type in an abandon
message and confirm the abandon.  This doesn't currently work well with the
code in PatchSetPanel, which is shared with the Submit button... I'll have to
sleep on it.
Sep 24, 2009
#9 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Wed Feb 25 07:00:24 PST 2009

Yea, the more we talk about this, the more it sounds like abusing
PatchSetPanel isn't a good idea.

Maybe instead add a "boolean canAbandon" to ChangeDetail, and use that in
PatchSetPanel's ensureLoaded(), just like populateCommentAction(), but
conditional on the canAbandon test?

We can then set the canAbandon boolean in ChangeDetailService when we
construct it.
Sep 24, 2009
#10 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Mon Mar 09 12:55:35 PDT 2009

Fixed by https://review.source.android.com/9085
Sep 24, 2009
#11 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Mon Mar 09 12:55:35 PDT 2009

Fixed in version 2.0.6.
Status: Fixed
Sep 25, 2009
#12 code-rev...@gtempaccount.com
(No comment was entered for this change.)
Labels: FixedIn-2.0.6
Jun 18, 2012
#13 amu...@wikimedia.org
For historical purpose, the commit sha1 is f6a57ac50379a7e68c4af5bc2a491ace74ddc66b
Oct 25, 2012
#14 sop@google.com
(No comment was entered for this change.)
Status: Released
Sign in to add a comment

Powered by Google Project Hosting