317021 - bz_canusewhines description should be improved

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[timeless]

currently: bz_canusewhines User can configure whine reports for self 

the 'for self' part is terrible.

Comment 1

[victory <[e-mail]>]

note for someone who notice this bug:
<timely> 'whine' really shouldn't be in the description at all
<timely> User can configure queries to be run at scheduled intervals and sent via email
<timely> the 'for self' part can be dropped entirely
<timely> bz_canwhineatothers can say
<timely> User can configure queries to be run at scheduled intervals and sent via email to other users or groups

Comment 2

[Matt Selsky [:selsky]]

Attachment: Clean up group descriptions, v1

Comment 3

[Gervase Markham [:gerv]]

Comment on attachment 8385886 [details] [diff] [review]
Clean up group descriptions, v1

I think bz_canusewhines needs to say specifically that the the reports can only be sent to that user. "sent to themselves by email"?

Gerv

Comment 4

[Matt Selsky [:selsky]]

For parallelism with the other description, how about "sent via email to themselves"?

Comment 5

[Gervase Markham [:gerv]]

Fine :-)

Gerv

Comment 6

[Frédéric Buclin]

Comment on attachment 8385886 [details] [diff] [review]
Clean up group descriptions, v1

>+        description  => 'Can configure queries and schedules for periodic reports to be run and sent via email to other users and groups',

>+        description  => 'Can configure queries and schedules for periodic reports to be run and sent via email',


These lines are way too long. They must be split.

Comment 7

[:glob ✱]

removing a? as per comment 6.

Comment 8

[Matt Selsky [:selsky]]

Attachment: Clean up group descriptions, v2

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 8413502 [details] [diff] [review]
Clean up group descriptions, v2

r=gerv by inspection.

Gerv

Comment 10

[Dave Miller [:justdave]]

Does this only change the defaults on a new install?  I'm guessing this doesn't change the existing ones.  Works for me anyway.

Comment 11

[Matt Selsky [:selsky]]

Correct, only the defaults are changed.  Do you want a patch for that as well?

Comment 12

[Dave Miller [:justdave]]

Given that this is a pre-defined group and is used for permissions, I think updating the existing description on an upgrade would be plausible as well.

Comment 13

[Dave Miller [:justdave]]

Since this hasn't been committed yet, let's go ahead and update it to do that before it gets committed.

Comment 14

[Gervase Markham [:gerv]]

Matt: looks like this is back with you :-)

Gerv

Comment 15

[Matt Selsky [:selsky]]

Attachment: v3

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 8516523 [details] [diff] [review]
v3

r=gerv.

Gerv

Comment 17

[Frédéric Buclin]

Comment on attachment 8516523 [details] [diff] [review]
v3

>+++ b/Bugzilla/Install/DB.pm

>+    $dbh->do('UPDATE groups SET description = ? WHERE name = ? and description = ?',
>+             undef,
>+             "Can configure queries and schedules for periodic reports to be run and sent via email to other users and groups",
>+             "bz_canusewhineatothers",
>+             "Can configure whine reports for other users");
>+    $dbh->do('UPDATE groups SET description = ? WHERE name = ? and description = ?',
>+             undef,
>+             "Can configure queries and schedules for periodic reports to be run and sent via email to themselves",
>+             "bz_canusewhines",
>+             "User can configure whine reports for self");

This is not the right place for this code. It must go into Bugzilla::Install::update_system_groups().

Comment 18

[Frédéric Buclin]

Also, I think these SQL commands are prone to problems because it doesn't take into account potential string changes. IMO, the right fix is to move the description out of the DB and into a template, as we do with user preferences (see global/setting-descs.none.tmpl). This way, it's very easy 1) to update strings, and 2) to localize them. But that's what bug 434381 is about.

So this bug should only fix the description for new installations, and moving descriptions from the DB into a template should be done in bug 434381.

Comment 19

[Matt Selsky [:selsky]]

Attachment: v4

I now use the API to update the group descriptions instead of raw SQL commands.

Comment 20

[Frédéric Buclin]

Comment on attachment 8517230 [details] [diff] [review]
v4

>+        else {
>+            if ($exists->description ne $definition->{description}) {

Combine else + if into a single elsif. Also, rename $exists to $group. That's much clearer which object we have in hands.

Otherwise looks good.

Comment 21

[Matt Selsky [:selsky]]

Attachment: v5

Comment 22

[Gervase Markham [:gerv]]

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

Review of attachment 8517435 [details] [diff] [review]:
-----------------------------------------------------------------

r=gerv.

Gerv

Comment 23

[Gervase Markham [:gerv]]

Oh, fiddlesticks.

To ssh://[e-mail]/bugzilla/bugzilla.git
   45439b7..5d8aa52  5.0 -> 5.0

My trunk checkout was switched to the 5.0 branch for some reason.

Is this a significant enough patch to back it out, or can we leave it?

Gerv

Comment 24

[Gervase Markham [:gerv]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   0eb6904..82674d4  master -> master

Gerv

Comment 25

[David Lawrence [:dkl]]

I do not see why this cannot be accepted for 5.0 as it is completed/ready and the issue was reported well before we branched. Feel free to override.

dkl

Comment 26

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #23)
> My trunk checkout was switched to the 5.0 branch for some reason.
> 
> Is this a significant enough patch to back it out, or can we leave it?

Due to comment 18, I would prefer to see it backed out of 5.0. But you cannot simply back out the patch because DB changes won't be reverted.

Comment 27

[Gervase Markham [:gerv]]

...and some (landfill only?) installs will have auto-updated to 5.0 and so if I backed out would require manual intervention?

Gerv

Comment 28

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #27)
> ...and some (landfill only?) installs will have auto-updated to 5.0 and so
> if I backed out would require manual intervention?

Right, or they would have a different group description than other 5.0 installations. Let's see what glob decides.

Comment 29

[Gervase Markham [:gerv]]

Oops. This is the right commit info for the master commit:

To ssh://[e-mail]/bugzilla/bugzilla.git
   34f2b77..88e4ee5  master -> master

Gerv

Comment 30

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #25)
> I do not see why this cannot be accepted for 5.0 as it is completed/ready
> and the issue was reported well before we branched. Feel free to override.

5.0 should only be for important bug fixes; taking any bug just because it was reported well before we branched is a good way of delaying 5.0's release further (_most_ of our bugs were reported before we branched!).

given the window is so small and the impact is only cosmetic, let's back this out of 5.0 and not worry about fixing up and updated descriptions.

(In reply to Gervase Markham [:gerv] from comment #29)
> Oops. This is the right commit info for the master commit:
> To ssh://[e-mail]/bugzilla/bugzilla.git
>    34f2b77..88e4ee5  master -> master

you've committed an older version of the patch.

please:
- revert the changes on the 5.0 branch
- revert the committed patch from master, and commit attachment 8517435 [details] [diff] [review]

Comment 31

[Gervase Markham [:gerv]]

Are there any other ways I could mess this up? Answers on a postcard to...

Backout on 5.0:

To ssh://[e-mail]/bugzilla/bugzilla.git
   5d8aa52..45877e0  5.0 -> 5.0

Backout on master and checkin of correct patch:

To ssh://[e-mail]/bugzilla/bugzilla.git
   88e4ee5..649a389  master -> master

Gerv

Comment 32

[Frédéric Buclin]

5 commits on trunk (including 2 backouts) just to commit this single patch?! Stop alcohol before doing checkins, seriously. Especially with patches which touch upgrade code. :(

It's a semi-joke. One day, the wrong code will be committed which corrupts data.

Comment 33

[Frédéric Buclin]

And I suppose this bug can be closed again?

Comment 34

[Gervase Markham [:gerv]]

I have learned a number of things from this bug, it must be said.

Gerv