Modify

Opened 11 years ago

Last modified 8 years ago

#11165 new defect

TracStatsPlugin does not properly count imported tickets

Reported by: achittur@… Owned by:
Priority: high Component: TracTicketStatsPlugin
Severity: critical Keywords:
Cc: Trac Release: 1.0

Description (last modified by Ryan J Ollos)

Change [13107] changed the results of the get_num_closed_tix() and get_num_open_tix() queries. The new version does not cooperate with batch-modified tickets. Neither version completely cooperates with tickets imported with status=closed via the TicketImport plugin.

Reproduction steps:

  1. Use TicketImport to import a set of bugs from CSV, some with open status and some with closed.
  2. Version [13106] of these methods do not correctly add the already-closed tickets to the new and closed statistics; there also appears to be a miscount. Version [13107] of these methods count the already-closed tickets as still-open.

Attempted failed workaround: Taking all the closed tickets, re-opening and re-closing them still does not get TicketStats to correctly count them.

Note: I am not sure it is entirely possible to get accurate statistics just by looking at the ticket_change table. The [13106] version of these queries look like they'll provide more accurate results. I believe it is far more important to users for these statistics to be accurate rather than fast.

Attachments (3)

ticketstats.py (9.3 KB) - added by achittur@… 11 years ago.
Partially reverted & corrected ticketstats.py
ticketstats_patch_11165.diff (6.0 KB) - added by achittur@… 11 years ago.
Patch file for quickfix
SQLiteManager_TicketChangeTable.png (83.9 KB) - added by Ryan J Ollos 11 years ago.

Download all attachments as: .zip

Change History (12)

Changed 11 years ago by achittur@…

Attachment: ticketstats.py added

Partially reverted & corrected ticketstats.py

comment:1 Changed 11 years ago by Ryan J Ollos

Could you please attach that as a patch, per t:TracDev/SubmittingPatches?

comment:2 Changed 11 years ago by Ryan J Ollos

Description: modified (diff)

comment:3 in reply to:  description ; Changed 11 years ago by Ryan J Ollos

Replying to achittur@…:

I believe it is far more important to users for these statistics to be accurate rather than fast.

Yes, any sane person would feel the same. The previous SQL queries were garbage. So perhaps there is a bug here now, but we'll aim to make them both fast and correct, and even better they will be readable and make sense.

I will be able to get this fixed faster if you send me the CSV file that you used to discover the error.

comment:4 Changed 11 years ago by achittur@…

I will resubmit as a patch. NOTE: I don't intend this to be final code, just a quickfix. The attached ticketstats.py is a temp-hacked version to get somewhat-correct statistics reporting again.

Summary of changes:

  1. get_num_closed_tix and get_num_open_tix in this version ONLY read from the ticket table, and infer open/closed states based on status, time, and changetime. While less than ideal for getting 100% accurate dates, it does now result in accurate counts, which is more important for my use case.
  2. The date loop calculation logic has been fixed - there were multiple bugs.
    1. Using datetime.now() returns local time, but the other calculations used UTC explicitly - this is changed to .utcnow() for consistency
    2. Fixed the values being passed to date_range in the for loop to gather data points
    3. By fixing (b), was able to remove the strange +12hrs being appended to at_date
    4. Changed date_str to be last_date, since that's actually the number a user wants to see
Last edited 11 years ago by Ryan J Ollos (previous) (diff)

Changed 11 years ago by achittur@…

Patch file for quickfix

comment:5 Changed 11 years ago by Ryan J Ollos

[13107] does not appear to make any modification to how datetimes are handled. Did you test that the datetime handling worked correctly before that changeset? The datetime handling in the plugin is done all wrong, and needs to utilize the Trac API in trac.util.datefmt. I've previously left #3934 open to handle this, but never got around to implementing it.

I'm not sure there is actually a bug in this program, but it might just be that the ticket you've imported aren't properly populating the ticket change table. I'm not willing to break the plugin code to handle your one special case, but maybe we can find a way to make it work for your case without breaking normal usage. Again, I'll need a CSV file to proceed.

comment:6 in reply to:  3 ; Changed 11 years ago by achittur@…

Replying to rjollos:

I will be able to get this fixed faster if you send me the CSV file that you used to discover the error.

Unfortunately I cannot send your my CSV, it contains proprietary information. I can tell you that it was simple & flat, NO histories or complicated layouts, and all the fields (like status) were pre-sanitized to Trac-friendly values.

From reading the code in ticketstats.py, one bug is that ticketstats was not interpreting ticket_change rows that came from batch modifications - batch modifies don't create new ticket_change rows for each ticket, there's only 1 row, but ticketstats was not taking that into account. Check out ticket/model.py save_changes where it changes existing ticket_change rows which implicitly came from the same batch-modify (or at least that's my interpretation of what's going on there).

There could also be a bug with the TicketImport plugin and the ticket_change table, but that's starting to get into Trac semantics which are well beyond my knowledge (I just started hacking Trac source last week and I'm rather new to Python).

Changed 11 years ago by Ryan J Ollos

comment:7 in reply to:  6 Changed 11 years ago by Ryan J Ollos

Replying to achittur@…:

Unfortunately I cannot send your my CSV, it contains proprietary information. I can tell you that it was simple & flat, NO histories or complicated layouts, and all the fields (like status) were pre-sanitized to Trac-friendly values.

Would it be possible to take just a few rows from your CSV file, sanitize the data and send that along? Or at least just send the column format?

From reading the code in ticketstats.py, one bug is that ticketstats was not interpreting ticket_change rows that came from batch modifications - batch modifies don't create new ticket_change rows for each ticket, there's only 1 row, but ticketstats was not taking that into account. Check out ticket/model.py save_changes where it changes existing ticket_change rows which implicitly came from the same batch-modify (or at least that's my interpretation of what's going on there).

What evidence do you have to support this claim? You are doing the batch modification from the Custom Query (/query) page? The reason I ask, is that there is an open defect for batch modification from the milestone and milestone admin panels (t:#5658), but anyway I'm not aware of a way to close tickets from the admin panel.

You are using the BatchModify module that is part of Trac 1.0, right, and not the old BatchModifyPlugin? The BatchModify module calls save_changes for each ticket, which will add new entries to the ticket_change table, see code here. Entries in the ticket_change table are not modified once they are added, except in a few corner cases such as the optional ticket deleter component and ticket edit feature, or by plugins such as TicketChangePlugin and TicketDeletePlugin (both obsolete by Trac 1.0).

I did a test just now to be sure, and here is what I see from SQLiteManager. You can see 3 entries for each batch-modified ticket: one for the comment, one for the status change and one for the resolution change.

comment:8 Changed 11 years ago by Ryan J Ollos

Summary: TracStatsPlugin Change 13107 does not properly count batch-modified ticketsTracStatsPlugin does not properly count imported tickets

I acknowledge your claim with regard to TicketImportPlugin. I'm not sure that I'll be doing anything about that, since your proposed changes make the behavior worse for non-imported tickets. Some of your claims for with regard to batch modified tickets are not correct, and I'm not convinced there is a bug for batch modified tickets. I'll do a bit more investigation though.

comment:9 Changed 8 years ago by Ryan J Ollos

Owner: Ryan J Ollos deleted

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.

Add Comment


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

 
Note: See TracTickets for help on using tickets.