Modify

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#6220 closed defect (fixed)

[patch] ticked owner user names considered invalid

Reported by: graham@… Owned by: farialima
Priority: high Component: TicketImportPlugin
Severity: critical Keywords: users import
Cc: Trac Release: 0.11

Description

I have created about a dozen users in the Trac project we have set up which are also listed on the "owner" column of a CSV file. When importing the file the plugin states that all the users are not valid users. All users have logged in at least once to the system before hand.

Attachments (2)

changeset_1330.diff (466 bytes) - added by rlrj60 4 years ago.
ticketimport_fix-existing-users.patch (922 bytes) - added by hasienda 4 years ago.
correctly add users as simple list members, not tuples to existingusers list

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by hoff.st@…

  • Summary changed from Users considered invalid to [more_info_needed] ticked owner user names considered invalid

If the owners definition is a comma separated list, this bug most likely is related to #5546.
So the reporter should provide an example of a problematic file, please.
Excel format was proven to help overcome some limitations of CSV. A complementary test importing same date from spreadsheet in XLS format could help as well.

comment:2 Changed 4 years ago by farialima

Yes, please check if it is caused by #5546 - I propose a workaround there. Otherwise a test file would be welcome indeed.

comment:3 Changed 4 years ago by hasienda

Sorry to say that, but 2 months without explicit request for more information, that suggests the issue is no longer relevant. I suggest to close this at least after another month without reply.

comment:4 Changed 4 years ago by farialima

Let's leave it opened a little longer... I haven't been very good either at answering timely to tickets :)... also I'd really like to see what's wrong. Again, a file (CSV or XLS) that shows the problem would be great.

comment:5 Changed 4 years ago by rlrj60

The issue was caused by the evaluation at Line #342 which always add any owner to the newusers list:

342 if cell  != '' and  cell not in newusers and  cell not in existingusers:
343     newusers += [ cell ]

Currently, existingusers wrongly contains a list of tuples, for example: [(u'user1,),(u'user2,),...] as supposed to a list of user names such as [u'user1, u'user2,...]. Therefore the statement cell not in existingusers always True.

This is because the function add_sql_result() appends a tuple to the list at line #289:

285 def add_sql_result(db, sql, list):
286    cursor = db.cursor()
287    cursor.execute(sql)
288    for result in cursor:
289       list += [ result ]    ###WRONG!!! add a tuple to list

To correct the issue, change the line to:

289       list += result        ###CORRECT!!! add a name to list

Changed 4 years ago by rlrj60

comment:6 Changed 4 years ago by hasienda

  • Summary changed from [more_info_needed] ticked owner user names considered invalid to [patch] ticked owner user names considered invalid

Oh, I see. Thanks for providing hint to root cause and bug fix right away - a developers dream. Patience did pay well here.

I checked and found the flaw in both, 0.10 and 0.11 branch. So closing this ticket should be a matter of applying a simple patch that I'll attach right now.

Changed 4 years ago by hasienda

correctly add users as simple list members, not tuples to existingusers list

comment:7 Changed 4 years ago by farialima

  • Status changed from new to assigned

comment:8 Changed 4 years ago by farialima

  • Resolution set to fixed
  • Status changed from assigned to closed

thanks for the patch. Applied in [9716]. Also get the list of users from the sessions, so that having logged is enough. Thanks for the implied suggestion :)

comment:9 Changed 3 years ago by hasienda

  • Keywords changed from users, import to users import

Sorry for the late follow-up, but I wasn't able to dedicate time to checking current plugin status sooner.

For this issue I've spotted, that you didn't applied the fix for the 0.10 branch, just for 0.11. It might just be personal preference (i.e. feeling 0.10 is no longer that relevant), but I don't know. While I personally would agree, I'd have applied the fix anyway, as it came for free, right? But it might still be disputable, like the obvious habit to disregard trunk and do all development and fixes on the named branch (0.11) instead. Just my 2 ct.

(OT: If I'll start i18n implementation as I spoke out loud some time ago, I'll definitely start to use the development branch again...)

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.