Improve API for getting comment number

[anonymous]

E.g. in ITicketManipulator.validate_ticket it is surprisingly difficult to get the comment number of the comment to be validated.

ticket.get_comment_number(ticket['changetime']) often gives the previous comment number, but fails e.g. when the a comment was edited. (​th:#12714)

Am I missing something or could the API be improved somehow? Thanks.

[Jun Omae]

You could use TicketModule.grouped_changelog_entries().

    def _get_ticket_cnum(self, ticket, modtime):
        for change in TicketModule(self.env) \
                      .grouped_changelog_entries(ticket, when=modtime):
            return change['cnum']

See tags/trac-1.0.10/trac/ticket/notification.py@:181-182,191#L165​.

[anonymous]

Unfortunately that also seems to fail when the last ticket modification was a comment edit. Or what should modtime be?

[anonymous]

Could something like this be added to Ticket?

    def get_ticket_max_comment_number(self):
        rows = self.env.db_query("""
            SELECT CAST(oldvalue AS INTEGER) as cnum
            FROM ticket_change
            WHERE ticket=%s AND field='comment'
            ORDER BY cnum DESC
            LIMIT 1
            """, (self.id,))
        return rows[0][0] if rows else 0

[Jun Omae]

Replying to anonymous:

Unfortunately that also seems to fail when the last ticket modification was a comment edit. Or what should modtime be?

The modtime is ticket_change.time column of the comment, not ticket['changetime'].

See when arguments of TicketModule.grouped_changelog_entries and Ticket.get_changelog.

[Jun Omae]

Your logic wouldn't work if the ticket is concurrently modified by several users. I think you should search comment matched with author by TicketModule.grouped_changelog_entries. I don't think it is needed to add that.

[anonymous]

The modtime is ticket_change.time column of the comment, not ticket['changetime'].

OK, but how do I get ticket_change.time in validate_ticket? If that was possible I could also simply use ticket.get_comment_number(ticket_change.time), no?

That's basically my problem: I can't find the latest comment_number because I can't find the latest ticket_change.time.

Your logic wouldn't work if the ticket is concurrently modified by several users.

The logic inside get_ticket_max_comment_number? Or the idea of "previous-comment-number + 1 = validation-comment-number"?

I think you should search comment matched with author by TicketModule.grouped_changelog_entries.

I'm afraid I don't understand. The author of the new comment?

def validate_ticket(self, req, ticket):
    author = get_reporter_id(req, 'author')    
    comment = req.args.get('comment')

    # Doesn't work after comment edits:
    comment_number_wrong1 = (ticket.get_comment_number(ticket['changetime']) or 0) + 1
    comment_number_wrong2 = self._get_ticket_cnum(ticket, ticket['changetime']) + 1

    # Seems to me this would work (except for concurrent modifications):
    comment_number_ok = ticket.get_ticket_max_comment_number() + 1

    # Your proposal?
    comment_number_by_author = self._search_comment_matched_with_author(ticket, author)

def _get_ticket_cnum(self, ticket, modtime):
    for change in TicketModule(self.env) \
                  .grouped_changelog_entries(ticket, when=modtime):
        return change['cnum']

def _search_comment_matched_with_author(self, ticket, author):
    for change in TicketModule(self.env) \
                  .grouped_changelog_entries(ticket):
        # ...?
        if change['author'] == author:
            return change['cnum']

Thanks you for your time.

[anonymous]

#13282 reported similar problems.

[Ryan J Ollos]

Regarding get_ticket_max_comment_number, I'll effectively address that in #13282. The Ticket.version field will be populated with the resource version, which is equivalent to the largest cnum, for the Ticket object associated with appending a comment to a ticket.

As for the problem you are addressing in this ticket, are you trying to get the cnum of the change that is about to be saved? As Jun mentioned, that's not possible due to concurrency. That's why I was describing how we might add apply_ticket_side_effects to the ITicketManipulator interface, which would be called after the ticket change has been saved.

You may be able to use ITicketChangeListener since it acts after the ticket change has been saved.

[Ryan J Ollos]

(Moved from #13282)

Replying to Ryan J Ollos:

are you trying to get the cnum of the change that is about to be saved

Yes.

Ticket.version field will be populated with the resource version, which is equivalent to the largest cnum

Even if there were attachments or comment edits (two ways to change the ticket without incrementing the cnum)?

If yes, good. So ticket.version+1 can (after #13282 only or already now?) be used to "guess" the cnum or the change that is about to be saved. (Ignoring concurrency.)

add apply_ticket_side_effects to the ITicketManipulator interface, which would be called after the ticket change has been saved

Yes, that could help. Hopefully it will be possible to detect "comment preview" vs. "new comment" vs. "edit comment" events?

Can apply_ticket_side_effects still modify the comment? I need ticket comment and PR ("side effect DB table") to be cross-referenced. So it would be best to save both at the same time, with the new cnum stored in the PR and the PR-index stored in the ticket comment.

That is also the reason ITicketChangeListener can not be used.

If apply_ticket_side_effects can not modify the ticket, it seems of very limited use, and maybe Trac should instead be changed to deal with the concurrency issue, so the manipulator has access to the transaction and cnum and changetime and so on:

• In a DB transaction:
  • lock tables
  • get new cnum
  • call manipulators
  • save change
  • commit transaction

It's probably a very difficult change, but I don't know. Maybe it is easy to refactor _process_ticket_request from:

self._validate_ticket(req, ticket, not valid)
#...
self._do_save(req, ticket, action)
#    _do_save internally calls:
#        now = datetime_now(utc)
#        cnum = ticket.save_changes(...)

to:

now = datetime_now(utc)
cnum = ticket.save_changes(..., only_get_new_cnum=True)
ticket.version = cnum 
self._validate_ticket(req, ticket, not valid)
self._do_save(req, ticket, action, now=now, cnum=cnum) # now and cnum are passed internally to ticket.save_changes

?

[Ryan J Ollos]

I've moved your comment from #13282. Let's keep discussion of your topic in this ticket. I'll reply later.

[Ryan J Ollos]

Replying to Ryan J Ollos:

Can apply_ticket_side_effects still modify the comment? I need ticket comment and PR ("side effect DB table") to be cross-referenced. So it would be best to save both at the same time, with the new cnum stored in the PR and the PR-index stored in the ticket comment.

You can append to the comment in ITicketManipulator.validate_ticket, and update the PR in ITicketChangeListener.{ticket_created/ticket_changed/ticket_comment_modified}, or update the PR in ITicketManipulator.apply_ticket_side_effects if you need access to the req to get the PR id?

Since ITicketManipulator.apply_ticket_side_effects doesn't exist yet, if you need access to fields from the Request you could add those to the ticket object in ITicketManipulator.validate_ticket and commit them to the database in ITicketChangeListener.

ITicketManipulator.validate_ticket is primarily document as being for identifying problems and rejecting changes, but it's location in the request processing makes it very useful for modifying the ticket changes.

I think it's been discussed before, ITicketChangeListener might be more useful if the request was passed as an argument. One way would be to move the call to ITicketChangeListener out of the model object. Also, it might be more user friendly if ticket changes were passed in a dict that included the cnum and cdate, rather than passing the ticket and old_values. But those are just ideas, maybe not even good ones, that won't happen soon.