95747 - typoed CC fails to abort before changes are made

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Dave Miller [:justdave]]

when adding a CC to a bug, if you typo an address, it tells you "this@address
isn't a bugzilla user.  Press Back and try again".  If you press back, and
correct your address, when you hit commit, it midairs, because the rest of the
changes you made to the bug applied anyway (but it didn't tell you they did).

The CC name to ID check needs to be run on the CC addresses before any changes
are written to the database.

Comment 1

[Dave Miller [:justdave]]

b.m.o shakedown

Comment 2

[Jacob Steenhagen]

Attachment: check cc list before touching bug

Comment 3

[Jacob Steenhagen]

What this patch does is moves part of the CC processing code out of the $id
loop.  This makes it so it can be run through DBIdToNameAndCheck() *before*
doing the other stuff with the bug [which is what it did before I broke it in
bug 55161 :( ].  Rather than processing everything twice, it sets up a hash of
what changes are requested outside the loop, then inside the loop it makes those
changes (if needed).

I also moved the declaration of my $removedCcString inside the loop so it has
the same scope of the processmail call (rather than global to the file).  This
hasn't caused any problems yet, but with the CC changes available on the mass
change page now (bug 12819) there's some possibilites.

Comment 4

[Jacob Steenhagen]

Oops... the patch has a stray # XXXXXX that I used to mark my place :(
Harmless, but not needed.

Comment 5

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

The patch looks good and works on my test installation, r=myk.

I like this approach (validate all data before committing any of it) a lot. 
Also, I couldn't find any problems with $removedCcString's new location.  It
looks like you have it in the right place.

Comment 6

[Jacob Steenhagen]

Commited.

Comment 7

[Dave Miller [:justdave]]

Moving to Bugzilla product

Comment 8

[Kevin Benton]

Jake - is there any way you can change this patch in such a way so that we don't
have two variables named cc_add - one hash and one scalar?  The same request for
cc_remove?  I'd like to see %cc_add renamed to %cc_add_list and %cc_removed to
%cc_removed_list.  That way, people who are reading this code won't get confused
as easily as I have.  If not, maybe we should consider opening a new bug to
rename those variables...  I know that Perl makes the distinction without any
problems, but I'm prone to mistaking one for the other.  I'm sure others may be
as well.

Comment 9

[Frédéric Buclin]

What you request is something new, so please open a new bug for that. We won't
back out a patch which landed almost 4 years ago.