TracStatsPlugin does not properly count imported tickets

[achittur@…]

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.

[achittur@…]

Partially reverted & corrected ticketstats.py

[Ryan J Ollos]

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

[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.

[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

[achittur@…]

Patch file for quickfix

[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.

[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).

[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.

[Ryan J Ollos]

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.