On Thu, Apr 08, 2010 at 06:59:39PM -0000, shypike wrote:
> shypike has proposed merging lp:~shypike/trac-launchpad-migrator/bugfixes-custom-mappings into lp:trac-launchpad-migrator.
> 
> Requested reviews:
>   Graham Binns (gmb)
> 
> 
> This change contains:
> 
> - Customizable mapping of users and milestones and nickname.
> 
> - Add xmlns attribute to <LaunchpadBugs> and <Bug>.
> 
> - Prevent crashes when encountering empty Trac fields.
> 
> - Convert [[br]] formatting in text fields to newlines.
> 
> - Map Trac-type "enhancement" to Lauchpad importance "Wishlist".
> 
> - Removed embedded "elisa" project customizations.
> 

Hi,

First, thanks for this branch. It's great to have people contributing
their code to this script.

There are a few issues with your branch that need fixing before we can
merge it with trunk. I've pointed these out below. Please reply to this
when you've made the changes I request and pushed them to Launchpad and
I'll happily do the merge for you.

Cheers,

Graham


> === modified file 'README'
> --- README	2009-06-17 14:46:38 +0000
> +++ README	2010-04-08 18:59:38 +0000
> @@ -18,6 +18,8 @@
>  Running the migration tool
>  --------------------------
>  
> +First customize the top part of the migrate.py script for your project.
> +
>  To create an XML export of a Trac ticket database, run the following
>  command:
>  
> 
> === modified file 'migrate.py'
> --- migrate.py	2009-10-20 10:31:04 +0000
> +++ migrate.py	2010-04-08 18:59:38 +0000
> @@ -23,12 +23,31 @@
>      print "You can install EaseXML by running `easy_install easexml`."
>      sys.exit()
>  
> -# This can be set to a hostname that will be used to create 
> -# email addresses from trac users that only have a username set but not 
> -# an email address. It should probably be set to the mailname of the 
> +#############################################################################################
> +# Start of customization
> +
> +# This can be set to a hostname that will be used to create
> +# email addresses from trac users that only have a username set but not
> +# an email address. It should probably be set to the mailname of the
>  # system that was running the trac instance.
>  TRAC_HOSTNAME = None
>  
> +
> +# Nickname to be used as a prefix of your TRAC issue numbers
> +# So TRAC bug 123 will result in Nickname-123
> +NICKNAME = 'name'

Thanks for fixing this one; it's been something I've needed to fix but
never got around to.

It's really important that this isn't just left as 'name', so I think it
should be None by default and make the script exit if it hasn't been
changed. What do you think?

> +
> +# Map TRAC aliases to real Lanchpad users
> +USER_ALIAS = {

For the sake of my sanity, this should be USER_ALIASES.

> +    'somebody' : 'user1',
> +    'anonymous' : 'user2',
> +}

Minor niggle, but our coding guidelines state that the closing brace of
a dict should be indented by 4 spacesi [1] (i.e. to line up with the contents
of the dict).

> +
> +# When reporters do not have valid account data
> +# set here which default user to use instead.
> +# If you leave this empty, a warning will be printed and the comment skipped.
> +DEFAULT_REPORTER = 'user1'
> +
>  # If you have a group of users whose details you already know, include
>  # them in the KNOWN_USERS dict in the following format to save time and
>  # database queries:
> @@ -38,7 +57,31 @@
>  #         'email': '<email address hidden>',
>  #         },
>  #     }
> -KNOWN_USERS = {}
> +KNOWN_USERS = {
> +     'user1': {
> +         'name': 'user1',
> +         'email': '<email address hidden>',
> +         },
> +     'user2': {
> +         'name': 'user2',
> +         'email': '<email address hidden>',
> +         }
> +}

I don't think it's necessary to provide example users here, since the
format is specified quite well in the comment above.

> +# Create mapping of milestones used in TRAC to milestones you have
> +# defined in Lauchpad.
> +MILESTONES = {
> +    # TRAC      Launchpad
> +    '0.1.0'   : '0.1.0' ,
> +    '0.1.1'   : '0.1.0' ,
> +    '0.2.0'   : '0.2.0' ,
> +    'future'  : ''
> +}
> +

Nice! However, I think it would be better to provide an empty MILESTONES
dict and explain the desired format in the comment rather than provide
the user with defaults that, if left unchanged, could break the import.
(Ideally of course we'd have a config file for the script rather than
using Python directly, but that's out of scope for this branch).

> +
> +# End of customization
> +#############################################################################################
> +
>  
>  RST_RE = r"""\{\{\{
>  #!rst
> @@ -50,13 +93,6 @@
>  EMAIL_RE = r"^[_\.0-9a-zA-Z-+=]+@(([0-9a-zA-Z-]{1,}\.)*)[a-zA-Z]{2,}$"
>  
>  
> -class LP(Namespace):
> -    uri = 'https://launchpad.net/xmlns/2006/bugs'
> -
> -    def getName(self):
> -        return ""
> -
> -
>  class Person(XMLObject):
>      _name = 'person'
>      username = StringAttribute(name='name')
> @@ -115,6 +151,7 @@
>  class Bug(XMLObject):
>      _name = 'bug'
>      id = StringAttribute()
> +    xlmns = StringAttribute(default='https://launchpad.net/xmlns/2006/bugs')

This should be 'xmlns' not 'xlmns'.

>      private = TextNode(optional=True)
>      security_related = TextNode(optional=True)
>      duplicateof = TextNode(optional=True)
> @@ -134,6 +171,7 @@
>  class LaunchpadBugs(XMLObject):
>      _name = 'launchpad-bugs'
>      _prettyPrint = False
> +    xlmns = StringAttribute(default='https://launchpad.net/xmlns/2006/bugs')

Here also.

>      bugs = ListNode('Bug')
>  
>  
> @@ -160,8 +198,9 @@
>              self._known_users_by_email[email] = new_data
>  
>      def get_user(self, username):
> +        username = USER_ALIAS.get(username, username)
>          user = {}
> -        is_email = username.find('@') > -1
> +        is_email = username and username.find('@') > -1
>          if is_email:
>              email = username
>  
> @@ -243,6 +282,7 @@
>      trac_priority = ticket['priority']
>      #trac_severity = ticket['severity']
>      trac_resolution = ticket['resolution']
> +    trac_type = ticket['type']
>  
>      priority_to_importance = {
>          'high': 'HIGH',
> @@ -255,6 +295,11 @@
>      importance = priority_to_importance.get(
>          trac_priority, 'UNKNOWN')
>  
> +    # LP doesn't have Defect/Enhancement distinction,
> +    # so map Enhancement to priority Whishlist
> +    if trac_type == 'enhancement':
> +        importance = 'WISHLIST'
> +
>      if trac_status == 'closed':
>          resolution_to_status = {
>              '': 'NEW',
> @@ -294,7 +339,11 @@
>      # Simply remove the {{{s and }}}s from the text. We don't want to
>      # care too much about blocks right now, so we just remove them.
>      block_re = re.compile('(\{\{\{|\}\}\})\r?\n')
> -    return block_re.sub('', text)
> +    text = block_re.sub('', text)
> +
> +    # Convert WikiFormatting for newlines
> +    text = text.replace('[[br]]', '\\n')
> +    return text

Nice, thanks.

>  
>  def ensure_valid_username(username):
> @@ -304,7 +353,7 @@
>  
>      if re.match(valid_name_re, username):
>          # If the name is already valid, return it.
> -        return username
> +        return USER_ALIAS.get(username, username)
>  
>      # Lowercase the username so that letters aren't treated as invalid.
>      username = username.lower()
> @@ -317,7 +366,7 @@
>          # that it becomes valid.
>          valid_name = valid_name + '1'
>  
> -    return valid_name
> +    return USER_ALIAS.get(valid_name, valid_name)
>  
>  
>  def extract_person_fullname(trac_person):
> @@ -370,7 +419,7 @@
>      #reporter_email = trac_reporter.get('email', '<email address hidden>')
>      reporter_email = extract_person_email(trac_reporter)
>      if not reporter_email:
> -        print '[ticket %s] pb with reporter %s' % (bug_id, reporter)
> +        print 'UNMAPPED REPORTER %s in ticket %s' % (reporter, bug_id)
>          return None
>  
>      reporter_name = extract_person_fullname(trac_reporter)
> @@ -387,8 +436,7 @@
>              trac_owner = db.get_user(owner)
>              owner_email = extract_person_email(trac_owner)
>              if not owner_email:
> -                print '[ticket %s] pb with owner %s' % (bug_id,
> -                                                        owner)
> +                print 'UNMAPPED OWNER %s in ticket %s' % (owner, bug_id)
>                  return None
>  
>              owner_name = extract_person_fullname(trac_owner)
> @@ -416,7 +464,7 @@
>  
>      contents = dict(id=str(bug_id),
>                      datecreated=datecreated,
> -                    nickname="elisa-%s" % bug_id,
> +                    nickname= "%s-%s" % (NICKNAME, bug_id),
>                      title=title,
>                      description=description,
>                      reporter=lp_reporter,
> @@ -427,23 +475,9 @@
>          contents['assignee'] = assignee
>  
>      if milestone:
> -        invalid = {'0.0.1 Guadec06 Preview': '0.0.1',
> -                   '0.0 bugfix': '0.0.1',
> -                   'elisa-0.3.3': 'core-0.3.3',
> -                   'testing': '',
> -                   'elisa-0.3.6': 'core-0.3.6',
> -                   'elisa-0.3.4': 'core-0.3.4',
> -                   'misc.win32.installer-0.1': '',
> -                   'elisa-0.3.5': 'core-0.3.5',
> -                   }
> -        try:
> -            milestone = invalid[milestone]
> -        except KeyError:
> -            pass
> -        else:
> -            if milestone:
> -                milestone = "elisa-%s" % milestone
> -                contents['milestone'] = milestone
> +        milestone = MILESTONES.get(milestone.strip())
> +        if milestone:
> +            contents['milestone'] = milestone
>  
>      bug = Bug(**contents)
>  
> @@ -463,13 +497,14 @@
>      #title = "%s ..." % description[:20]
>  
>      reporter = trac_comment['author']
> -    if reporter is None:
> -        return None
> +    if not reporter:
> +        reporter = DEFAULT_REPORTER
>      trac_reporter = db.get_user(reporter)
> +    if not trac_reporter:
> +        trac_reporter = db.get_user(DEFAULT_REPORTER)
>      reporter_email = extract_person_email(trac_reporter)
>      if not reporter_email:
> -        print '[comment %s] pb with comment reporter %s' % (1,
> -                                                            reporter)
> +        print 'UNMAPPED COMMENTER %s in comment' % (reporter, )
>          return None
>      reporter_name = extract_person_fullname(trac_reporter)
>      sender = Sender(
> 

 [1] https://dev.launchpad.net/PythonStyleGuide#Multiline%20braces