138546 - Need email CC addition option on create attachment page

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Felix Miata]

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.

Comment 1

[Matthew Tuck [:CodeMachine]]

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.

Comment 2

[Dave Miller [:justdave]]

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.

Comment 3

[Dave Miller [:justdave]]

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)

Comment 4

[Wayne Mery (:wsmwk)]

related Bug 320363

Comment 5

[Daniel Brooks [:db48x]]

Attachment: 138546-1.diff

This fixes it. Testing is available via my bugzilla instance on landfill, http://landfill.bugzilla.org/bug369429/

Comment 6

[Daniel Brooks [:db48x]]

Attachment: 138546-2.diff

better fix. adds the cc change to the bug's activity log. also fixes bug 320363.

Comment 7

[Frédéric Buclin]

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.

Comment 8

[Daniel Brooks [:db48x]]

Attachment: 138546-3.diff

Comment 9

[Frédéric Buclin]

I don't like the use of eval {} when we don't need them.

Comment 10

[Daniel Brooks [:db48x]]

Then squint your eyes and pretend it's a try { … } catch { } instead. It's the same thing.

Comment 11

[Frédéric Buclin]

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

Comment 14

[jag (Peter Annema)]

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

Comment 15

[Denis Roy]

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

Comment 16

[Reed Loden [:reed]]

Attachment: patch - v4

Three years later, this is way simpler to do. :)

Untested. Will request review once I test.

Comment 17

[Reed Loden [:reed]]

Attachment: patch - v5

Made a few layout changes, but the patch works.

Comment 18

[Max Kanat-Alexander]

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?

Comment 19

[Reed Loden [:reed]]

Attachment: patch - v6

Comment 20

[Max Kanat-Alexander]

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.

Comment 21

[Reed Loden [:reed]]

Attachment: patch - v7

Nits addressed.

Comment 22

[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

Comment 23

[Max Kanat-Alexander]

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?

Comment 25

[Max Kanat-Alexander]

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.

Comment 26

[Frédéric Buclin]

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

Comment 28

[Frédéric Buclin]

reed: ping? Could you update your patch? Else reassign the bug so that someone else can do it for you.

Comment 29

[Matt Selsky [:selsky]]

Attachment: v8

Updated version of Reed's patch.

Comment 30

[Frédéric Buclin]

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.

Comment 31

[Frédéric Buclin]

Attachment: patch, v8.1

Here is the patch I'm going to commit, with all my comments addressed.

Comment 32

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/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.

Comment 33

[Frédéric Buclin]

Added to relnotes for 4.4.