Closed Bug 97729 Opened 23 years ago Closed 22 years ago

Uploaders need to be able to obsolete their own attachments

Categories

(Bugzilla :: Attachments & Requests, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: justdave, Assigned: bbaetz)

References

Details

(Whiteboard: [permissions:edit])

Attachments

(2 files, 4 obsolete files)

Users who have uploaded attachments should be able to mark the obsolete flag on
attachments they uploaded.
OS: Mac System 8.5 → All
Priority: -- → P1
Hardware: Macintosh → All
Target Milestone: --- → Bugzilla 2.16
*** Bug 97731 has been marked as a duplicate of this bug. ***
*** Bug 98111 has been marked as a duplicate of this bug. ***
*** Bug 98508 has been marked as a duplicate of this bug. ***
Whiteboard: [permissions:edit]
Attached file Sorry! Test
Should this apply to all statuses?
if by other statuses, you mean first-review and second-review, probably not. 
Possibly they should have access to the needs-work status.
*** Bug 118127 has been marked as a duplicate of this bug. ***
Depends on: 102957
I don't think we should discriminate between statuses. Sometimes someone says
"Fix X and Y, and r=gerv", and the person should be able to upload the fixed
version and check the first-review box.

However, I really don't understand why anyone would want to upload an attachment
to immediately mark it obsolete. "Obsolete" means "no-one should ever have to
care about this again" - if that's true, why upload it?

Gerv
Because someone might say needs-work, and then they want to obsolete it when
they do the next version?

needs-work is a customisable status, so you can't restrict based on that. I
think we should allow the attacher to change anything on the attachment.

Possibly we should let the qacontact edit all attachments, but I don't think its
worth the effort.

In fact, the edit attachment screen shouldn't even appear for users who can't
edit the attachment, since they can't do anything with it. The link then
shouldn't appear on the show_bug page either, but that should wait for the
templatisation stuff, I think.

Taking.
Assignee: myk → bbaetz
Attached patch patch (obsolete) — Splinter Review
OK, try this. I did change the show_bug page in the end, since this is done by
Attachment.pm which is already templatised.

I also needed to add some push/pop stuff to the UserInGroup command. This stuff
should be cached like params are, but thats a separate bug which I'll file now.


(The removal of ValidateBugID from the obsolete stuff is OK, because we've
already done that once before getting that far, and we check that the
attachment is from the same bug. In other words, it was an unneeded check.)
Attached patch new patch (obsolete) — Splinter Review
Actually, I've changed my mind - the caching isn't needed, so use a local var
instead. I'll keep the push/pop in there though
Attachment #68087 - Attachment is obsolete: true
Comment on attachment 68088 [details] [diff] [review]
new patch

Looks good, works fine.  Two nits:

>Index: attachment.cgi

>+  validateCanEdit($::FORM{'id'});

Use $::FORM{'id'} instead of passing a parameter for consistency with the
other validation routines (or change the other routines to do it this way).


>+    # Bug 97729 - the submitter can edit their attachments
>+    SendSQL("SELECT submitter_id FROM attachments WHERE " .
>+            "attach_id = $attach_id AND submitter_id = $::userid");
>+
>+    (FetchOneColumn() == $::userid) ||
>+      DisplayError("You are not authorised to edit attachment #$attach_id") &&
>+        exit;

You are doing twice as much work here as necessary.  Either select the
attachment by attach ID and compare submitter_id to $::userid or select 
boolean true by attach ID and user ID and check if a column was returned.
Attachment #68088 - Flags: review-
> Use $::FORM{'id'} instead of passing a parameter for consistency with the
> other validation routines (or change the other routines to do it this way).

Can't do that, because the obsoleting routine passes in the attachment to be
obsoleted, which is different each time (and $FORM::{'id'} isn't set yet.

I'll fix the other issue tomorrow.
>Can't do that, because the obsoleting routine passes in the attachment to be
>obsoleted, which is different each time (and $FORM::{'id'} isn't set yet.

D'uh, you are right.  Ok, never mind. :-)
Attached patch v3 (obsolete) — Splinter Review
I had to clear the fetchahead buffer manually. Yuck - any better solutions?
Attachment #68088 - Attachment is obsolete: true
>I had to clear the fetchahead buffer manually. Yuck - any better solutions?

Yes, instead of:

+    (MoreSQLData()) ||
+      DisplayError("You are not authorised to edit attachment #$attach_id") &&
+        exit;

Do:

+    FetchSQLData() ||
+      DisplayError("You are not authorised to edit attachment #$attach_id") &&
+        exit;

Style nit: the Perl style guide Bugzilla follows says to break up long lines
after operators but before the ||/&&/and/or operators and to line up similar
items vertically like so:

    FetchSQLData() 
      || DisplayError(...)
      && exit;
Won't that give me a warning involving undefined vars if there isn't any data?
(And it will be false if the value returned is 0, but thats not a problem here,
I guess)
Keywords: patch, review
Attached patch v4 (obsolete) — Splinter Review
OK, it doesn't cause a warning. Try this.
Attachment #68129 - Attachment is obsolete: true
I personally dislike that the "Edit" link will disappear. "Edit Attachment As
Comment" and "Comment (on the bug)" make still sense, even if one cannot edit
the bug.
Yeah, I thought about that. However, I'd guess that most people reviewing a
patch would have canedit, and thats the main reason to edit attachment as
comment. Allowing access to part of teh screen is more complicated, because
you'd have to check that no changes were made to the atthment statuses, or any
part of the patch itsself. Its not impossible, its just a pain, and we're trying
to wrap up 2.16.

This can be revisited when this screen is rewritten (bug 102957)
Status: NEW → ASSIGNED
Comment on attachment 69189 [details] [diff] [review]
v4

>Index: attachment.cgi
> [snip]
>+  validateCanEdit($::FORM{'id'});
> [snip]
>+  validateCanEdit($::FORM{'id'});
> [snip]
>+sub validateCanEdit
> [snip]
>+    ValidateCanEdit($attachid);

One of these isn't like the rest (feel like you're reading a little kids book?
:)

>Index: globals.pl

Everything that was in globals.pl in this patch has been checked in already as
part of bug 110013.
Attachment #69189 - Flags: review-
Nope, thats correct. When editing an attachment, we only check that one (passed
in via the form). When creating an attachment, we don't have the new one to
check - we check all the existing ones. See comment 13.
Oops, I didn't elaberate like I intended to... the difference is that in the
first three, it's validateCanEdit (lower case 'v') and in the last one, it's
ValidateCanEdit (upper case 'v').  This results in a run-time error when
checking one of the obsolete checkboxes on the create attachment page (undefined
sub).
Attached patch v5Splinter Review
Oops.
Attachment #69189 - Attachment is obsolete: true
Comment on attachment 71121 [details] [diff] [review]
v5

>+  my $in_editbugs = &::UserInGroup("editbugs");

The usual name for such a variable is $canedit. And do you need the &:: ?

>+    $a{'canedit'} = ($::userid == 0 || $submitter_id == $::userid ||
>+                     $in_editbugs);

"canedit" should be a $vars->{'canedit'} and not a a property of the attachment
IMO.

>+    # Bug 97729 - the submitter can edit their attachments

You don't need to quote the bug number. That's what CVS blame is for :-)

Other than that, can't see any other issues. But I'll need to test it.

Gerv
Attachment #71121 - Flags: review-
err. You missed the point with that patch. My point is that canedit _is_ a per
attachment behaviour so that people can only obsolete their own attachments, not
someone elses
in)editbugs is called once, because useringroup doesn't cache so we don't want
to call it once per attachment
You need &:: because its in a module

cvs blame is a pain as soon as someone else changes adjacent lines. Its helpful
to quote the bug# when doing something which isn't immediately obvious (like,
say, naming a field Bugzilla_Password instead of oldpassword... ;)
> Its helpful
> to quote the bug# when doing something which isn't immediately obvious

I'm not saying remove the explanatory comment, just the bug number :-) But
anyway, I withdraw my other comments, but I still can't give this review until
I've tested it.

Gerv

Comment on attachment 71121 [details] [diff] [review]
v5

Gerv withdrew his comments that caused the needs-work, so removing that.

Every looks good to me, r=jake
Attachment #71121 - Flags: review- → review+
removing dependency since the overall editattachment redo got pushed to 2.18,
and this is too needed to push it off (plus it has a patch :)
No longer depends on: 102957
Comment on attachment 71121 [details] [diff] [review]
v5

r= justdave

it works.

My only concern is it also allows uploaders to change any status on their own
patch.	Depending on what those statuses are that could be a good thing though.
 It'll have to be the reviewer's responsibility to make sure the correct people
set the statuses. :-)

Check it in!
Attachment #71121 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
*** Bug 143772 has been marked as a duplicate of this bug. ***
Component: Creating/Changing Bugs → attachment and request management
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: