Modify

Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#6220 closed defect (fixed)

[patch] ticked owner user names considered invalid

Reported by: graham@… Owned by: François Granade
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 14 years ago.
ticketimport_fix-existing-users.patch (922 bytes) - added by Steffen Hoffmann 14 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 14 years ago by Steffen Hoffmann

Summary: Users considered invalid[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 14 years ago by François Granade

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 14 years ago by Steffen Hoffmann

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 14 years ago by François Granade

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 14 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 14 years ago by rlrj60

Attachment: changeset_1330.diff added

comment:6 Changed 14 years ago by Steffen Hoffmann

Summary: [more_info_needed] ticked owner user names considered invalid[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 14 years ago by Steffen Hoffmann

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

comment:7 Changed 13 years ago by François Granade

Status: newassigned

comment:8 Changed 13 years ago by François Granade

Resolution: fixed
Status: assignedclosed

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 13 years ago by Steffen Hoffmann

Keywords: users, importusers 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...)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain François Granade.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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

 
Note: See TracTickets for help on using tickets.