| Issue 41: | Cannot abandon a change | |
| Back to list |
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
Sep 24, 2009
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
Update by Shawn Pearce <sop@google.com> on Wed Feb 18 09:10:55 PST 2009 Assigned to Brad Larson.
Sep 24, 2009
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
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
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
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
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
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
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
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
(No comment was entered for this change.)
Labels:
FixedIn-2.0.6
Jun 18, 2012
For historical purpose, the commit sha1 is f6a57ac50379a7e68c4af5bc2a491ace74ddc66b
Oct 25, 2012
(No comment was entered for this change.)
Status:
Released
|
|
| ► Sign in to add a comment |