Edgewall Software
Modify

Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#11018 closed defect (fixed)

Deleting a milestone != default milestone

Reported by: ahaferburg@… Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.3
Component: roadmap Version: 0.12.4
Severity: normal Keywords: milestone
Cc: Branch:
Release Notes:

Empty strings are stored as NULL in the database for fields of the Ticket class.

API Changes:

Description

We ended up deleting one of our milestones. The tickets are now displayed as milestone "(empty)". Judging from the SQL their milestone is NULL. New tickets, however, where the milestone dropdown box is left at the default value, are filed under "Milestone ", and I guess this means that the milestone is the empty string, "".

That means new tickets end up in a different milestone than the tickets from the deleted milestone, and I don't think there is any way to change the milestone from NULL to "".

Attachments (2)

Report3.png (29.0 KB ) - added by Ryan J Ollos 5 years ago.
t11018.patch (2.0 KB ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Christian Boos, 6 years ago

Component: web frontendroadmap
Keywords: milestone verify added
Milestone: next-stable-1.0.x

in reply to:  description comment:2 by Christian Boos, 6 years ago

Milestone: next-stable-1.0.x1.0.2

Reproduced.

Right, it seems better to be consistent here. However, looking at the code (blame), it seems it was done on purpose in r7581. So it looks like the inconsistency is due to a change from NULL to "" when setting the milestone to "None" in the ticket editor.

That means new tickets end up in a different milestone than the tickets from the deleted milestone, and I don't think there is any way to change the milestone from NULL to "".

Changing it to any other milestone, then back again to the empty entry does the trick. But as mentioned above, better leave them to NULL until we solve that for new tickets.

I hesitate a bit for the milestone. It seems fine to fix this for 1.0.2 but 1.1.2 would give us the possibility to "fix" the "" values during an upgrade.

comment:3 by Christian Boos, 6 years ago

As mentioned elsewhere, we have the empty value to help us not introduce spurious diffs with '' vs None, so it seems indeed fine to fix that for 1.0.2 without touching to the '' values already in the db.

comment:4 by Ryan J Ollos, 5 years ago

Owner: set to Ryan J Ollos
Status: newassigned

I'll see about working up a patch for this one as well.

by Ryan J Ollos, 5 years ago

Attachment: Report3.png added

comment:5 by Ryan J Ollos, 5 years ago

I struggled to figure out where any negative effects of the NULL vs empty string are seen. Eventually I found a difference in Report 3.

Does the problem manifest itself anywhere else in Trac? I imagine this ambiguity could cause a number of unforeseeable problems for plugins and various other customizations as well.

The issue seems to be fixed already for ticket properties retrieved through a Ticket object (which calls Ticket._fetch_ticket). In that case, the milestone property (and other ticket fields) will always be returned as an empty string, regardless of whether the database value is an empty string or null. However, a report such as Report 3 that runs a direct SQL query will continue to see differences between the empty string and null in the database.

I looked at 4 cases:

  1. Creating a ticket when the milestone table is empty. Ticket.insert iterates over the fields and sets the values in the database, however since the milestone table is empty the field is treated as if it didn't exist. The milestone is therefore left as null.
  2. Creating a ticket when the milestone table is not empty. The default milestone is the empty string.
  3. Creating a ticket when all of the milestones are closed. The milestone is forced to the default milestone and therefore the empty string is set.
  4. When a milestone is deleted or closed and the tickets are retargetted to the None milestone. When the milestone is deleted and tickets retargetted to None, the milestone will be set to null after [7581]. However, if the milestone is closed and ticket retargetted to None, the milestone will be set to the empty string.

It looks to me like this only needs to be fixed at the database level since _fetch_ticket will already replace nulls with empty strings when retrieving ticket values. We just need to consistently store the data using whatever is chosen to represent the empty value. Due to (1), using the empty string as the empty value in the database would be a bit difficult. Since unset values in the database will remain null, it will be much easier to obtain consistency in the database by replacing empty strings with None before inserting into the ticket table. This might be accomplished by a patch as simple as the following (but needs more testing first):

  • trac/ticket/model.py

    diff --git a/trac/ticket/model.py b/trac/ticket/model.py
    index 15d09aa..b43d221 100644
    a b class Ticket(object):  
    238238            cursor.execute("INSERT INTO ticket (%s) VALUES (%s)"
    239239                           % (','.join(std_fields),
    240240                              ','.join(['%s'] * len(std_fields))),
    241                            [values[name] for name in std_fields])
     241                           [values[name] or None for name in std_fields])
    242242            tkt_id = db.get_last_id(cursor, 'ticket')
    243243
    244244            # Insert custom fields
    class Ticket(object):  
    246246                db.executemany(
    247247                    """INSERT INTO ticket_custom (ticket, name, value)
    248248                       VALUES (%s, %s, %s)
    249                     """, [(tkt_id, c, self[c]) for c in custom_fields])
     249                    """, [(tkt_id, c, self[c] or None) for c in custom_fields])
    250250
    251251        self.id = tkt_id
    252252        self.resource = self.resource(id=tkt_id)

So a possible solution is to do the following on 1.1.x:

  • Apply the patch above.
  • Create an upgrade script to replace existing empty string values in the database with null.
Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:6 by Ryan J Ollos, 5 years ago

Milestone: 1.0.2next-stable-1.0.x
Owner: Ryan J Ollos removed
Status: assignednew

I'm moving this forward since I'm not sure what to do about comment:5 yet. I'll continue working the ticket if I get some input on the direction to take, or become more confident on deciding which direction to take.

comment:7 by loomchild, 5 years ago

Your solution works, it will keep running the patched version and report any potential problems.

comment:8 by Ryan J Ollos, 5 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:9 by loomchild, 5 years ago

I see one problem.

  1. I have a ticket with assigned milestone, let's say 'M1'.
  2. I modify the ticket milestone and set it to empty string in dropdown list.
  3. I go to Active Tickets by Milestone report and again I see a single ticket in a new section "Milestone" in addition to (empty). This ticket's milestone field is set to an empty string instead of NULL.

by Ryan J Ollos, 5 years ago

Attachment: t11018.patch added

comment:10 by Ryan J Ollos, 5 years ago

I overlooked the need for modifications to Ticket.save_changes, in addition to the changes made to Ticket.insert. t11018.patch should fix the issue described in comment:9.

We'll also need to decide if empty strings should be replaced with nulls in the ticket_change table. I haven't made any modifications to ticket_change yet.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:11 by loomchild <pub@…>, 5 years ago

Great, patch works. Thanks for working on this.

As for the table I manually replace empty strings with nulls and I haven't encountered any issues yet. I will keep testing.

comment:12 by Ryan J Ollos, 5 years ago

Milestone: next-stable-1.0.x1.0.3

comment:13 by Ryan J Ollos, 4 years ago

Looks like it might be better to add a _to_db_types method and do the ''None replacement of values in the dictionary in a single location. Ticket._to_db_types(...) was already added on trunk in [11331#file1].

comment:14 by Ryan J Ollos, 4 years ago

Keywords: verify removed

comment:15 by Ryan J Ollos, 4 years ago

Proposed changes in log:rjollos.git:t11018 address the issue for the Ticket model. Work in #11419 is leading to some significant refactoring that will fix the issue for the other model classes of the Ticket system. In #11419 I'll propose an upgrade step that will selectively replace empty strings with NULLs in the database.

comment:16 by Ryan J Ollos, 4 years ago

Looking at [11331#file1], log:rjollos.git:t11018 should replace db_values[name] with db_values.get(name).

comment:17 by Ryan J Ollos, 4 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [13546], merged to trunk in [13547].

comment:18 by Ryan J Ollos, 4 years ago

The issue reported in #11891 was discovered while working on this ticket.

comment:19 by Ryan J Ollos, 3 years ago

TracDev/ScratchPad/DataModels@4#Standardizemissingvalues was updated to account for changes made in this ticket.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.