Is that a case that we need "revoked" items with an event?

In such case you can just pass event_id and you'll get revoked ones. In case you would more refined data, maybe history search?

yeah, a condition "%(table)screate_event <= %(event)d" % locals() which is similar with query_history. It doesn't look necessary to be covered by eventCondition

The largest issue I see here is that we're also calling get_tag_extra with the same event parameter. The tag_extra table is multivalued at a given event, so we simply can't use the same logic there. At the moment, we're going to get every tag extra value ever set in the deleted tag case.

I believe we'll need to pass the correct event id to get_tag_extra

Other notes:

• we shouldn't add the order option in the original case
• typo: if event != 'inactive':
• we're losing the the single value sanity check in the original case

Given that eventCondition() alone can't return a condition that accomplishes what we want, I'm not sure it make sense to make a change there at all. The calling code already has to have very different logic in the revoked case; it can just skip adding the condition too. TRUE IS TRUE isn't really an event condition.

I think the difficulty in finding a clear name for the special event value might be an indication that this is the wrong approach. It might be better to go the other way and add a new option to the call.

That said, perhaps event = "auto"?

I've cleaned it a lot (and dropped policy part as it needs more work according to #2917).

The event query doesn't seem to have any clauses?

MIght be cleaner to add revoke_event to fields in the auto case rather than selectively delete it after.

Ouch, updated.

I've not checked that correct revoke_event is returned in local test. I'll add tests when we settle the behaviour here.

I don't think we're picking the right event. We're getting the most recent create_event, but that might not be the most recent event for the followup tag_extra query.

I think we want to:

• query revoke_event in this first query (but still taking the entry with highest create_event)
• if revoke_event is null, then the tag is not deleted and we should go with event = None
• otherwise we should go with event = revoke_event - 1

The delete_tag() function does everything in one transaction, so all the revokes from that should have the same event id (and even if they did advance, the tag_config entry is revoked first). So, pretty sure revoke_event - 1 is the best representation of the undeleted tag.

Also, the code fails with event=auto and a nonexistent tag. Easiest thing to do is set event=None when we don't get a row and fail later (I think using the singleValue method with strict=False will do this in one line), but we could also duplicate the error code.

updated

Thanks! LGTM!

Note that this fixes #1506 but not #2917

Commit e7b6e78f fixes this pull-request