97729 - Uploaders need to be able to obsolete their own attachments

Status: RESOLVED FIXED

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

Description

[Dave Miller [:justdave]]

Users who have uploaded attachments should be able to mark the obsolete flag on
attachments they uploaded.

Comment 1

[Dave Miller [:justdave]]

*** Bug 97731 has been marked as a duplicate of this bug. ***

Comment 2

[Dave Miller [:justdave]]

*** Bug 98111 has been marked as a duplicate of this bug. ***

Comment 3

[Jacob Steenhagen]

*** Bug 98508 has been marked as a duplicate of this bug. ***

Comment 4

[Yuriy Pavlenko]

Attachment: Sorry! Test

Comment 5

[Matthew Tuck [:CodeMachine]]

Should this apply to all statuses?

Comment 6

[James Henstridge]

if by other statuses, you mean first-review and second-review, probably not. 
Possibly they should have access to the needs-work status.

Comment 7

[Jacob Steenhagen]

*** Bug 118127 has been marked as a duplicate of this bug. ***

Comment 8

[Gervase Markham [:gerv]]

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

Comment 9

[Bradley Baetz (:bbaetz)]

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.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: patch

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.)

Comment 11

[Bradley Baetz (:bbaetz)]

Attachment: new patch

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

Comment 12

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 13

[Bradley Baetz (:bbaetz)]

> 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.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

>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. :-)

Comment 15

[Bradley Baetz (:bbaetz)]

Attachment: v3

I had to clear the fetchahead buffer manually. Yuck - any better solutions?

Comment 16

[Myk Melez [:myk] [@mykmelez]]

>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;

Comment 17

[Bradley Baetz (:bbaetz)]

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)

Comment 18

[Bradley Baetz (:bbaetz)]

Attachment: v4

OK, it doesn't cause a warning. Try this.

Comment 19

[Tobias Burnus]

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.

Comment 20

[Bradley Baetz (:bbaetz)]

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)

Comment 21

[Jacob Steenhagen]

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.

Comment 22

[Bradley Baetz (:bbaetz)]

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.

Comment 23

[Jacob Steenhagen]

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).

Comment 24

[Bradley Baetz (:bbaetz)]

Attachment: v5

Oops.

Comment 25

[Gervase Markham [:gerv]]

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

Comment 26

[Bradley Baetz (:bbaetz)]

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... ;)

Comment 27

[Gervase Markham [:gerv]]

> 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 28

[Jacob Steenhagen]

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

Comment 29

[Dave Miller [:justdave]]

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 :)

Comment 30

[Dave Miller [:justdave]]

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!

Comment 31

[Bradley Baetz (:bbaetz)]

Checked in.

Comment 32

[Bradley Baetz (:bbaetz)]

*** Bug 143772 has been marked as a duplicate of this bug. ***