71552 - Remove oldemailtech from Bugzilla

Status: RESOLVED FIXED

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

Description

[Dave Miller [:justdave]]

I could swear we had a bug for this already, but I can't find it, so I'm making 
one.

Comment 1

[Dave Miller [:justdave]]

Making this a 2.14.  It's not exactly security, but bugzilla.mozilla.org can't 
easily cvs update again until this is yanked.  Regenerating the shadow directory 
(the ASCII copies of all the bugs in ${BUGZILLA_HOME}/shadow that oldemailtech 
uses to make the diffs it mails out) on running checksetup.pl takes approximately 
7 hours to run on b.m.o with 71000 bugs.

Comment 2

[Dave Miller [:justdave]]

this is a test...

Comment 3

[Mark Olson]

helping endico test

Comment 4

[Jason Kersey]

Testito.  Looks good so far, altho the vers is still 2.11....

Comment 5

[Mark Olson]

i didn't follow instructions the first time, commenting again

Comment 6

[Jacob Steenhagen]

What exactly is the purpose of having it regenerate the shadow files in
checksetup?  I'd imagine it has something to do w/making sure that new fields
get added to the shadow files at update instead of at the first change of the
bug (being that the files already exist simply by the fact that the bug was
changed).  Being that oldmail is on it's way out, wouldn't it be possible to
simply pluck that routine from checksetup.pl for 2.14 (possibly the first thing
done in 2.13)?  We could then save the real work of removing oldmail for 2.16.

Dont' get me wrong, I have no love for the oldmail system and would prefer to
see it gone ASAP, but it just seems like it be better to wait for 2.16.

Comment 7

[Matthew Tuck [:CodeMachine]]

Marking this dependent on a bug that says emailsuffixes and newemailtech are
mutually exclusive.  I'm not sure if that will disappear along with this, but we
can't break Bugzilla features.

Comment 8

[Jacob Steenhagen]

OK, I can take this one and make it my #1 priority... I don't have a whole lot
going on this week, so it should be done very soon.

What I have on my list of things to do:
 * Remove code that supports oldmail from:
    - userprefs.cgi
    - processmail
    - checksetup.pl (namely, processmail regenerate)

 * Remove the following params:
    - newemailtech
    - newemailtechdefault
    - changedmail

 * Add a routine to checksetup to remove no longer needed stuff:
    - shadow/*
    - profiles.emailnotification
    - profiles.newemailtech

Known issues:
 * Currently when somebody vists userprefs.cgi?bank=diffs for the first time it
   retrieves their current setting from profiles.emailnotification and if it's
   ExcludeSelfChanges it flags that option in the profiles.emailflags field.
   While it's possible to do that in checksetup.pl before removing the field, I
   personally don't think it's worth it.  If I'm wrong, I can add that
   functionality to the checksetup.pl routine.
 * When removing a param from defparams.pl, it never removes it from
   data/params... probably not a big deal, but worth mentioning

I'd also like an opionion on how to check for the existance of oldmail... Should
it be if the shadow directory exists, or if profiles.newemailtech is in the DB?

Comment 9

[Matthew Tuck [:CodeMachine]]

I suggest you don't have just one check for old emailtech.  Remove shadow if
shadow exists, remove the params if they exist, etc.  That way you can cope with
various situations like the script dying half way through.

Comment 10

[Jacob Steenhagen]

Attachment: PATCH to remove oldemailtech

Comment 11

[Jacob Steenhagen]

Lot's of "-" in this patch :)

Things changed since my last comment:
 * Discovered that Param("prettyasciimail") was no longer needed
 * Added globals.pl and editusers.cgi to the list of files to remove code from

Notes about this patch:
 * Added support for data/nomail to the newemailtech code
 * There are almost no whitespace changes even though I removed "if" statements
   and came accross other code that isn't use "4 space indenting".  This was
   mostly to make it easier to see what changed.
 * I removed the "CC: %cc%" line from the newchangedmail default param.  CC
   isn't used by newemailtech anyway.  I did leave the code in processmail
   otherwise the existance of "%cc%" in the param will cause processmail to fail
 * I'm 95% sure that the Lock and Unlock subs were there because of the file
   level diffing, so I removed them.
 * Rather than simply removing the "regenerate" option from processmail, I
   changed it so it says "Regenerating is no longer required or supported"
 * In userprefs.cgi I removed some (all?) of the "NEW!!" indications.

Comment 12

[Zach Lipton [:zach]]

Can this be tested on landfill?

Comment 13

[Tara Hernandez]

No.  Landfill is only for red-haired people with green eyes.

Silly.

Okay, patch is up.  Let the testing commence.

Comment 14

[Terry Weissman]

I strongly suggest putting the whitespace changes in, so you don't end up with a
file having illegible indentation.

"cvs diff -b" can be used to show the differences without seeing the annoying
whitespace-only changes.

Comment 15

[Gervase Markham [:gerv]]

While you are in the email system, any chance of you making CC field diffs a bit 
more intelligent? It would be _really_ handy for bugmail just to say who had 
been added and who had been removed. If this is too complex, can you at least 
change the code order so CC field changes appear at the bottom of bugmail?

Gerv

Comment 16

[Jacob Steenhagen]

Attachment: PATCH w/whitespace changes

Comment 17

[Jacob Steenhagen]

Gerv... I do plan on changing that behaviour at some point, but it's not part of
this bug.  Bug 61015 is currently flagged for 2.16.  I have some ideas on how to
do that differently, but think that focusing on getting 2.14 out the door is the
higher priority right now...  ;)

Comment 18

[Jacob Steenhagen]

Attachment: patch v2 - Unbreak editusers.cgi

Comment 19

[Jacob Steenhagen]

I discovered that I missed a routine (user creation) in editusers.cgi...
"patch v2" also contains whitespace changes...

Comment 20

[Dave Miller [:justdave]]

Attachment: The required code to add to checksetup.pl to remove the params that are no longer being used

Comment 21

[Dave Miller [:justdave]]

patch v2 has been running on bugzilla.syndicomm.com for two weeks now and we 
haven't even noticed the difference.  Which is a good thing for this particular 
patch. :-)

r= justdave

Although I think we ought to add the code to remove the params, that could 
probably be another bug.

Comment 22

[Jacob Steenhagen]

Checked in.  Bug 82497 has been created for the removal of the stranded params.

Comment 23

[Dave Miller [:justdave]]

Moving to Bugzilla product