Roundup Tracker - Issues

Issue 2550576

classification
Split up _handle_message in roundup/mailgw.py
Type: Severity: normal
Components: Mail interface Versions: 1.4
process
Status: closed accepted
:
: schlatterbeck : jerrykan, schlatterbeck
Priority: : patch

Created on 2009-08-18 03:40 by jerrykan, last changed 2011-01-18 10:07 by schlatterbeck.

Files
File name Uploaded Description Edit Remove
mailgw.py.patch jerrykan, 2010-10-18 00:44
mailgw.py.newpatch schlatterbeck, 2010-12-21 21:53 new patch against original mailgw.py
Messages
msg3852 Author: [hidden] (jerrykan) Date: 2009-08-18 03:40
It would be nice if _handle_message in roundup/mailgw.py was split up
into many functions instead of one large one. I imagine it would have
benefits for maintainabilty, but...

I have a custom mailgw script that inherits from MailGW, and I am
currenly defining my own _handle_message to change parts of how emails
are handled as they come in. For the most part it is identical to the
original _handle_message, but is does contain a few small alterations.

I am basically duplicating ~600 lines of code just the change a few
small parts of it. If _handle_message was split up into many functions,
it could override only the ones I want to change, without having to
duplicate large chunks of code (and help improve compatability with
future versions).
msg4154 Author: [hidden] (jerrykan) Date: 2010-10-18 00:44
Quite a large patch to split the _handle_message function up into
multiple smaller functions and decouple some of the code in the process.

The evolution of this patch can be viewed at:
  http://gitorious.org/roundup/roundup/commits/mailgw
msg4212 Author: [hidden] (schlatterbeck) Date: 2010-12-21 21:53
Hello John,
I really like the idea of splitting up the mail parsing.
I've taken a closer look at the patch.
I like the way you semantically dissected the large method into smaller
ones.
There are some (small) errors so I've not (yet) accepted it as-is.

The following corrections would be necessary:
- _handle_message_get_pgp_message currently defines the function
  pgp_roles after using it some lines above
  there currently is no regression test for the pgp feature... I'm not
  using it myself.
- the db.commit that commits a user is called two times, once in the
  method building the author _handle_message_get_author_id and once in
  the top-level _handle_message routine. The call in the
  _handle_message_get_author_id method is wrong, it commits before some
  other checks are done.
- some values are recomputed several times. This may introduce
  hard-to-find bugs in the future (and *decreases* maintainability)
  because a change might fix one computation and miss another. 
  One example is the recomputation of message.extract_content in two
  separate methods (and only using one of the two returned values)

When refactoring this I'd introduce a new class, lets call it
"parsedMessage". In this I'd put all the _handle_* methods as class
methods of the new class (stripping the prefix _handle_message). Then
the constructor of the new class would call all your _handle methods and
build the data structures. Finally a method committing all this to the
database would be called by the original _handle_message routine.
Maybe the intermediate commit would also be taken out of the __init__.
This would also avoid lots of passed arguments: Each of the new methods
will introduce some object variables as self.<variable>, e.g., we would
compute the class only once as self.cl (bad example, this *is* later
explicitly recomputed when the database is reopened for the new user)
Maybe we can even add this to the currently existing Message class.

If you have time I'd appreciate if you again work on that patch.
Alternatively I might look into this more closely but this might take
more time.... I'd probably take the route of committing the fixed patch
(attached) and working from there to factor the code into a separate class.

I'm attaching a modified patch that fixes the first two issues above.

Thanks for this work
Ralf
msg4213 Author: [hidden] (jerrykan) Date: 2010-12-23 05:01
Hello Ralf,

Thanks for taking to time to look at this. It has been a while since
I've looked at the code, so I'll need to reacquaint myself with it :D

I am happy to have to have another look at the patch in the (hopefully)
not too distant future. It is somewhat dependant on work commitments,
but I'll let you know how I go.

Cheers,
John.
msg4214 Author: [hidden] (schlatterbeck) Date: 2010-12-23 15:46
Thanks John -- I had some time now and used your code to factor the
_handle_message code into a separate class. This avoids recomputing
values and in particular the parameter-passing. Otherwise this is quite
identical to your refactoring.
I had to fix the bugs mentioned earlier, added some more tests for
things that were not covered (message id generation) and have also fixed
the parameter parsing (-C and -S options) to work with other classes not
named "issue" (hopefully, for this there is no test yet).

Fixed in r4577
msg4215 Author: [hidden] (jerrykan) Date: 2010-12-24 01:14
Hello Ralf,

I was hoping to have another look at the patch and provide some
additional feedback about the patch before anything was committed
(unfortunately holiday session has already kicked in though).

I have some reservations about about the 're-factoring into a separate
class'. The initial aim of this patch (for me at least) was so that it
would make it easier to alter parts of the MailGW for specific cases
user may have.

A simple example might be that I don't want to send a help message back
to the user. Using my initial patch I could achieve this by having my
own script that would be something like:

 # import roundup stuff here
 class OurMailGW(MailGW):
   def _handle_message_help(self, message):
       return
 
 # define 'instance' and 'mailbox' here
 gw = OurMailGW(instance, {})
 gw.do_mailbox(mailbox)

But with the approach of using a separate class, I would need to:
 * create my own parsedMessage class (OurParsedMessage)
 * override the 'handle_help' function in OurParsedMessage
 * create my own MailGW class (OurMailGW)
 * override the '_handle_message' and duplicate the entire contents just
so I can change the following line:
     parsed_message = OurParsedMessage(self, message)

This second approach seems a lot less maintainable, and less intuitive.
The size of the MailGW class may be reduced a bit by moving the code out
of it, but I'm not sure it introduces some significant drawbacks.
msg4221 Author: [hidden] (schlatterbeck) Date: 2011-01-10 16:07
John,
First a good new year to you.
Thanks for the feedback.

You are raising a valid point here, one of the goals of the refactoring
is easier override of the message parsing and that isn't achieved with
my change.

I still think that it's a good idea to put all the state during message
parsing into a separate object for maintainability reasons.

I've now introduced in r4578 a class variable of MailGW named
parsed_message_class that by default points to the parsedMessage in
mailgw.py, you still have to override both classes but the code will not
duplicate existing long methods. With that change your use-case becomes:

 # import roundup stuff here
 class OurParsedMessage(parsedMessage):
   def handle_help(self):
       return

 class OurMailGW(MailGW):
   parsed_message_class = OurParsedMessage
 
 # define 'instance' and 'mailbox' here
 gw = OurMailGW(instance, {})
 gw.do_mailbox(mailbox)

Can you live with that?

Ralf
msg4222 Author: [hidden] (jerrykan) Date: 2011-01-11 02:06
Hello Ralf,

I haven't had a chance to play with the new changes, but at a quick
glance it seems to be a good solution and one I am happy with (it would
only require a few minor changes to my existing script). If I happen to
notice anything else down the track, I can always create a new issue. So
I would consider this issue "done" and thanks for helping to get the
changes committed.

Cheers,
John.
msg4227 Author: [hidden] (schlatterbeck) Date: 2011-01-18 10:07
On Tue, Jan 11, 2011 at 02:06:38AM +0000, John Kristensen wrote:
> 
> John Kristensen <John.Kristensen@dpipwe.tas.gov.au> added the comment:
> 
> Hello Ralf,
> 
> I haven't had a chance to play with the new changes, but at a quick
> glance it seems to be a good solution and one I am happy with (it would
> only require a few minor changes to my existing script). If I happen to
> notice anything else down the track, I can always create a new issue. So
> I would consider this issue "done" and thanks for helping to get the
> changes committed.

Thanks for the feedback, let me know if you find something to fix when
you have time to look into it.

Ralf
History
Date User Action Args
2011-01-18 10:07:57schlatterbecksetmessages: + msg4227
2011-01-11 02:06:38jerrykansetmessages: + msg4222
2011-01-10 16:07:47schlatterbecksetmessages: + msg4221
2010-12-24 01:14:12jerrykansetmessages: + msg4215
2010-12-23 15:46:27schlatterbecksetstatus: new -> closed
assignee: schlatterbeck
resolution: accepted
messages: + msg4214
2010-12-23 05:01:37jerrykansetmessages: + msg4213
2010-12-21 21:53:05schlatterbecksetfiles: + mailgw.py.newpatch
nosy: + schlatterbeck
messages: + msg4212
2010-10-18 00:44:27jerrykansetfiles: + mailgw.py.patch
keywords: + patch
messages: + msg4154
2009-08-18 03:40:11jerrykancreate