Closed Bug 138546 Opened 20 years ago Closed 10 years ago

Need email CC addition option on create attachment page

Categories

(Bugzilla :: Attachments & Requests, enhancement)

2.15
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: mrmazda, Assigned: selsky)

References

Details

Attachments

(1 file, 8 obsolete files)

If on first visit to a bug I choose to create an attachment, I get no email CC.
I have to go back to bug to do no more that add the CC.
We can't add all the fields to this page and I don't see why this one is special.

We really need to integrate attaching onto the main page, in a similar way to
how advanced query works, and then we can do actions at the same time as any
number of attachments and close out a lot of enhancement requests.
Target Milestone: --- → Future
Component: Bugzilla-General → attachment and request management
Reassigning all of my "future" targetted bugs to indicate that I'm not presently
working on them, and someone else could feel free to work on them.
Reassigning all of my "future" targetted bugs to indicate that I'm not presently
working on them, and someone else could feel free to work on them. (sorry for
the spam if you got this twice, it didn't take right the first time)
Assignee: justdave → nobody
related Bug 320363
QA Contact: mattyt-bugzilla → default-qa
Target Milestone: Future → ---
Attached patch 138546-1.diff (obsolete) — Splinter Review
This fixes it. Testing is available via my bugzilla instance on landfill, http://landfill.bugzilla.org/bug369429/
Assignee: nobody → db48x
Status: NEW → ASSIGNED
Attachment #265917 - Flags: review?(myk)
Attached patch 138546-2.diff (obsolete) — Splinter Review
better fix. adds the cc change to the bug's activity log. also fixes bug 320363.
Attachment #265917 - Attachment is obsolete: true
Attachment #265926 - Flags: review?
Attachment #265917 - Flags: review?(myk)
Blocks: 320363
Attachment #265926 - Flags: review? → review?(myk)
Comment on attachment 265926 [details] [diff] [review]
138546-2.diff

>Index: attachment.cgi

>+    if ($cgi->param('addselfcc'))
>+    {
>+        $dbh->do(q{INSERT INTO cc (bug_id, who) VALUES (?, ?)}, undef, $bugid, $user->id);
>+        LogActivityEntry($bugid, "cc", "", $user->login, $user->id, $timestamp);
>+    }

On race conditions, you will try to insert the user twice in the CC table.


>+  $vars->{'bug'} = my $bug = new Bugzilla::Bug($attachment->bug_id, Bugzilla->user->id);

You don't need 'my $bug' here. Moreover, if you create a bug object, do it a few lines earlier in the code so that you can remove the existing SQL query.


>+    if ($cgi->param('addselfcc'))
>+    {
>+        $dbh->do(q{INSERT INTO cc (bug_id, who) VALUES (?, ?)}, undef, $bugid, $user->id);
>+        LogActivityEntry($bugid, "cc", "", $user->login, $userid, $timestamp);
>+    }

Same comment as above about race conditions.
Attachment #265926 - Flags: review?(myk) → review-
Attached patch 138546-3.diff (obsolete) — Splinter Review
Attachment #265926 - Attachment is obsolete: true
Attachment #265987 - Flags: review?(LpSolit)
I don't like the use of eval {} when we don't need them.
Then squint your eyes and pretend it's a try { … } catch { } instead. It's the same thing.
Comment on attachment 265987 [details] [diff] [review]
138546-3.diff

As said on IRC, we don't like this usage of eval {}. Maybe this is because we are close minded? ;)
Attachment #265987 - Flags: review?(LpSolit) → review-
Duplicate of this bug: 365490
Duplicate of this bug: 454096
I'm not familiar with this code, but it seems to me that adding the necessary logic to deal with the race becomes much more verbose, where using eval to trap the exception and skip over adding an entry to the activity table seems a reasonably elegant solution. You could add some sort of test that checks $@ for any error and then warn or otherwise handle conditions other than "trying to insert a duplicate entry into the cc table".
I know this is currently an enhancement request, but I'd like to bump it up as a bug if I understand this bug correctly.

If a user's preferences are to "Automatically add me
to the CC list of bugs I change:Only if I have no role on them" then I think adding/modifying an attachment qualifies as 'changing a bug'.
Attached patch patch - v4 (obsolete) — Splinter Review
Three years later, this is way simpler to do. :)

Untested. Will request review once I test.
Assignee: db48x → reed
Attachment #265987 - Attachment is obsolete: true
Target Milestone: --- → Bugzilla 3.8
Attached patch patch - v5 (obsolete) — Splinter Review
Made a few layout changes, but the patch works.
Attachment #452319 - Attachment is obsolete: true
Attachment #452382 - Flags: review?(mkanat)
Attachment #452382 - Flags: review?(mkanat) → review-
Comment on attachment 452382 [details] [diff] [review]
patch - v5

This looks generally fine (although it needs clearance from pyrzak), but since this is technically still trunk, could you add a bug.user_has_any_role() method to Bugzilla::Bug instead of the has_role stuff in the templates?
Attached patch patch - v6 (obsolete) — Splinter Review
Attachment #452382 - Attachment is obsolete: true
Attachment #456016 - Flags: review?(mkanat)
Attachment #456016 - Flags: review?(guy.pyrzak)
Comment on attachment 456016 [details] [diff] [review]
patch - v6

>+    if (Bugzilla->params->{'useqacontact'}
>+        && ($self->{'qa_contact'} && $self->{'qa_contact'} == $user->id)) {

  Nit: perlstyle is to align the { with the "i" in "if", for multi-line if statements.

  Also, technically you don't need those extra parens, because all your conditions are &&.


  Otherwise, I'm generally happy to have this function, but I don't feel too comfortable with the UI as it is. Experience tells me that it's probably best to leave the UI review aspect to pyrzak, though, who is more likely to have good suggestions on what to actually do with it.
Attachment #456016 - Flags: review?(mkanat) → review-
Attached patch patch - v7 (obsolete) — Splinter Review
Nits addressed.
Attachment #456016 - Attachment is obsolete: true
Attachment #456232 - Flags: review?(mkanat)
Attachment #456232 - Flags: review?(guy.pyrzak)
Attachment #456016 - Flags: review?(guy.pyrzak)
Comment on attachment 456232 [details] [diff] [review]
patch - v7

I'm not going to bother commenting on the CGI/Perl because presumably that's what mkanat will be doing.
>=== modified file 'template/en/default/attachment/create.html.tmpl'
>--- template/en/default/attachment/create.html.tmpl	2010-03-03 06:13:28 +0000
>+++ template/en/default/attachment/create.html.tmpl	2010-07-07 07:37:03 +0000
>@@ -94,6 +94,7 @@
>         </td>
>       </tr>
>     [% END %]
>+

nit. do we need a new line here?

>     <tr>
>       <th><label for="comment">Comment:</label></th>
>       <td>
>@@ -106,6 +107,15 @@
>           cols    = constants.COMMENT_COLS
>           wrap    = 'soft'
>         %]
>+        [% IF NOT bug.cc || NOT bug.cc.contains(user.login) %]
>+          <br>
>+          <input type="checkbox" id="addselfcc" name="addselfcc"
>+            [% " checked=\"checked\""
>+                 IF user.settings.state_addselfcc.value == 'always'
>+                    || (!bug.user_has_any_role()
>+                        && user.settings.state_addselfcc.value == 'cc_unless_role') %]>
>+          <label for="addselfcc">Add me to CC list</label>
>+        [% END %]
>       </td>
>     </tr>
>     [% IF user.is_insider %]
>
>=== modified file 'template/en/default/attachment/edit.html.tmpl'
>--- template/en/default/attachment/edit.html.tmpl	2010-03-03 15:36:45 +0000
>+++ template/en/default/attachment/edit.html.tmpl	2010-07-07 07:37:03 +0000
>@@ -256,8 +256,17 @@
>               wrap    = 'soft'
>               classes = classNames          
>             %]
>+            [% IF NOT attachment.bug.cc || NOT attachment.bug.cc.contains(user.login) %]
>+              <input type="checkbox" id="addselfcc" name="addselfcc"
>+                [% " checked=\"checked\""
>+                     IF user.settings.state_addselfcc.value == 'always'
>+                        || (!attachment.bug.user_has_any_role()
>+                            && user.settings.state_addselfcc.value == 'cc_unless_role') %]>
>+              <label for="addselfcc">Add me to CC list</label>
>+            [% END %]
>           </div>
>-        [% END %]             
>+        [% END %]
>+             
>         <div id="attachment_flags">
>           [% IF attachment.flag_types.size > 0 %]
>      
>

So these 2 sets of code are the same except for how you access the bug object. IMO they should be moved to a separate file and called from there. However, I'll let mkanat be the final authority on that

Other than that it looks good to me. I'm going to R+ it and let mkanat be the final judge on these 8 blocks
Attachment #456232 - Flags: review?(guy.pyrzak) → review+
Comment on attachment 456232 [details] [diff] [review]
patch - v7

This looks great to me, but I agree with pyrzak's comment about unifying the code blocks. It looks actually almost like those three blocks are nearly identical--seems like they should be in a separate file, no?
Attachment #456232 - Flags: review?(mkanat) → review-
Duplicate of this bug: 581446
Target Milestone: Bugzilla 4.0 → Bugzilla 4.2
Comment on attachment 456232 [details] [diff] [review]
patch - v7

This looks fine to me, might as well check it in. If there are cleanups we can do them later, for this.
Attachment #456232 - Flags: review- → review+
Flags: approval+
Comment on attachment 456232 [details] [diff] [review]
patch - v7

>=== modified file 'Bugzilla/Bug.pm'

>+sub user_has_any_role {

This method should be merged with user(). This way, we would have only one central place to check user privs and roles. $bug->user_has_any_role would then be $bug->user->has_any_role.

I would like this change to be made before this patch is checked in (and the new patch attached here).
Duplicate of this bug: 665718
Flags: approval+ → approval?
Target Milestone: Bugzilla 4.2 → Bugzilla 5.0
reed: ping? Could you update your patch? Else reassign the bug so that someone else can do it for you.
Attached patch v8 (obsolete) — Splinter Review
Updated version of Reed's patch.
Assignee: reed → selsky
Attachment #456232 - Attachment is obsolete: true
Attachment #630840 - Flags: review?(LpSolit)
Comment on attachment 630840 [details] [diff] [review]
v8

Seems to work fine, but there are several things to fix before checking in the patch.


>=== modified file 'Bugzilla/Bug.pm'

>+    my $hasanyrole = $isreporter || $user->id == $self->{'assigned_to'} ||
>+        (Bugzilla->params->{'useqacontact'} && $self->{'qa_contact'} && $self->{'qa_contact'} == $user->id);

$hasanyrole should be renamed to $has_any_role, which is much more readable. Also, the code to check if the user is the assignee or QA contact should be refactored a bit to avoid duplicated code. New variables $is_assignee and $is_qa_contact should be created so that they can be reused.


>+                       hasanyrole => $hasanyrole};

s/hasanyrole/has_any_role/



>=== modified file 'template/en/default/attachment/create.html.tmpl'

>         %]
>+      [% IF NOT bug.cc || NOT bug.cc.contains(user.login) %]

The indentation is wrong.


>+            IF user.settings.state_addselfcc.value == 'always' ||
>+               (!bug.user.hasanyrole &&

Nit: || and && should be at the beginning of their respective lines, to follow our guidelines.



>=== modified file 'template/en/default/attachment/edit.html.tmpl'

>+                     (!attachment.bug.user_has_any_role() &&

Must be attachment.bug.user.has_any_role (old method name still used here).


r=LpSolit with these comments fixed.
Attachment #630840 - Flags: review?(LpSolit) → review+
Attached patch patch, v8.1Splinter Review
Here is the patch I'm going to commit, with all my comments addressed.
Attachment #630840 - Attachment is obsolete: true
Attachment #636718 - Flags: review+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified attachment.cgi
modified Bugzilla/Bug.pm
modified template/en/default/attachment/create.html.tmpl
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/bug/edit.html.tmpl
Committed revision 8268.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: approval? → approval+
Keywords: relnote
Resolution: --- → FIXED
Flags: testcase?
Added to relnotes for 4.4.
Keywords: relnote
Duplicate of this bug: 881037
You need to log in before you can comment on or make changes to this bug.