86051 - Cannot change capitalisation of products

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P3)

Description

[GavinS]

This is spun off from bug#64192

Because the comparison is case-insensitive when checking for existing component
and product names, if you try and change the capitalisation of a component from,
say 'test' to 'TEST', bugzilla says the component already exists.

Comment 1

[Matthew Tuck [:CodeMachine]]

Attachment: Fix.

Comment 2

[Matthew Tuck [:CodeMachine]]

The above patch will probably clash with the patch on bug #64192.

Comment 3

[Matthew Tuck [:CodeMachine]]

Attachment: Same for products.

Comment 4

[Matthew Tuck [:CodeMachine]]

Attachment: Let's try it again. (Combined Patch)

Comment 5

[Matthew Tuck [:CodeMachine]]

Moving to new Bugzilla product ...

Comment 6

[Luis Villa]

r=[e-mail]

clean patch and solves a problem I've noticed here once before.

Comment 7

[Dave Miller [:justdave]]

Comment on attachment 40274 [details] [diff] [review]
Let's try it again. (Combined Patch)

marking first-review on behalf of louie

Comment 8

[Dave Miller [:justdave]]

reassigning to patch author

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 40274 [details] [diff] [review]
Let's try it again. (Combined Patch)

>+        if (lc($component) ne lc($componentold)) {
>+            if (TestComponent($product,$component)) {

>+        if (lc($product) ne lc($productold)) {
>+            if (TestProduct($product)) {

I know it's a silly nitpick, but no use introducing more confusion, so we might as well
fix it now. :-)  When you edit a product or component, and *don't* change the name, it
still claims that you updated the product name, even when you didn't.  This is likely to
confuse some people.  Maybe try doing something like:

if (($product ne $productold) && (lc($product) ne lc($productold))) {

Comment 10

[Dave Miller [:justdave]]

kiko, I know you like short patches, want a stab at this one?

Comment 11

[Christian Reis]

Sure, if you can explain what you need me to do - this seems kindof fixed to me
with MattyT's patch.

Comment 12

[Dave Miller [:justdave]]

read my last comment. :)  It tells you that you changed the component when you
didn't.

Comment 13

[Gervase Markham [:gerv]]

kiko, mattyt: either of you planning to finish this off?

Gerv

Comment 14

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 15

[Christian Reis]

I can't, Gerv; perhaps MattyT is better off right now? :/

Comment 16

[GavinS]

I had a look at this patch, and, with it attached to 2.14:

1) Dave: I couldn't get it to say it had changed the name when I hadn't done
so,  so I don't think the patch needs work in that respect. (comment#9)

2) If Bugzilla is trying to be as helpful as possible, then it should probably
say 'No changes made', if none were.

3) I'm not sure that the patch is the correct way to go. I think the problem is
with the TestProduct() and TestComponent() calls. With the patch applied, and
you try and create a new product named 'TEST', when you already have a product
named 'test', you get the same problem as when changing the capitalisation.

Is it possible to change the SQL comparison to be case-sensitive? I think you
can do this by changing the data type, but I'd hope that wouldn't be necessary.

Comment 17

[GavinS]

I've updated the title, because I think the same issue arises (on 2.14 anyway)
for the keywords and groups :(

I can raise new bugs for these, if that would be easier?

I also tried a different fix to the currently attached patch for the original
component/product issue. This used SQL to select all the components which
matched the new component name and then Perl to check, case-sensitively, for
matches.

This approach worked OK for adding new components whose name only differed in
capitalisation to existing component names, and for changing capitalisation,
but, of course, you then have problems selecting which component you want to edit :(

If you want Bugzilla to be able to have 2 components whose names only differ in
capitalisation, then I think that the bug for using component_ids instead of
component names needs to be fixed before this bug can be. (can't remember the
bugid for that one at the mo.)

Comment 18

[Dave Miller [:justdave]]

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

Comment 19

[GavinS]

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

Comment 20

[Dave Miller [:justdave]]

Taking Jake's bugs...  his Army Reserve unit has been deployed.

Comment 21

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 22

[GavinS]

Attachment: Allow product capitalisation changes

This just deals with products. It allows product capitalisation changes, and
also gives an error when a new product differs only in case from an existing
one

Comment 23

[Jouni Heikniemi]

Comment on attachment 150303 [details] [diff] [review]
Allow product capitalisation changes

>Index: editproducts.cgi
>+    # Check for exact case sensitive match:
>+    if ("$existing_product" eq "$product") {

Nit: kill the quotes around the variables; they're really unnecessary.

>+    if (lc("$existing_product") eq lc("$product")) {

Same here, although I'd be ready just do this latter test; it's quite ok to say
"The product name 'xxx' is already in use" even if there is a case-only
difference. Humans don't consider that a difference.

>+        if (lc($product) ne lc($productold)) {
>         if (TestProduct($product)) {
>             print "Sorry, product name '$product' is already in use.";
>             SendSQL("UNLOCK TABLES");
>             PutTrailer($localtrailer);
>             exit;
>         }
>+        }

Combine the conditions using && instead of this unindented nested if.

Comment 24

[GavinS]

Attachment: Allow product capitalisation changes (V2)

Comment 25

[GavinS]

(In reply to comment #23
> Nit: kill the quotes around the variables; they're really unnecessary.

Done (too much bash programming!)

> 
> >+    if (lc("$existing_product") eq lc("$product")) {
> 
> Same here, although I'd be ready just do this latter test; it's quite ok to say
> "The product name 'xxx' is already in use" even if there is a case-only
> difference. Humans don't consider that a difference.

I've kept the test, if that is OK -- as I'm sure there will be one day when someone tries this and 
complains if we don't do this test.

> Combine the conditions using && instead of this unindented nested if.

Done

Comment 26

[Jouni Heikniemi]

Comment on attachment 153340 [details] [diff] [review]
Allow product capitalisation changes (V2)

r=jouni

Comment 27

[Jouni Heikniemi]

GavinS: File separate bugs on Components, Keywords and Groups; mention the bug
ids here so that people can track the discussion. Thanks!

Comment 28

[GavinS]

Created bug#252003, bug#252004, and bug#252005 for the components, keywords and groups issues

Comment 29

[Jouni Heikniemi]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.55; previous revision: 1.54
done

Thanks again, Gavin!