AutoQuery as a patch to trac

[jhammel@…]

The ​th:AutoQueryPlugin, which provides links to trac's query interface based on field name and value in the ticket box, may be implemented as a simple patch to trac.

see ​http://groups.google.com/group/trac-dev/msg/9bd18f28e6ff930b for discussion of the issue

The original ticket is ​th:#3226

Patch attached

[jhammel@…]

patch to trac trunk r7249

[osimons]

Patch seems to be committed to trunk in [7499], but I have some minor problems with it.

The patch conflicts with the efforts to provide e-mail obfuscation as fields like authorinfo(ticket.reporter) (and owner) that did obfuscation depending on permissions are now links that naturally must link to the true value to be meaningful.

At the very least, EMAIL_VIEW permissions should be checked before rendering links for owner and reporter. Additionally 'owner' and 'reporter' could be default for fields to not link-ify. On the subject of defaults, I don't quite see why ['estimatedhours', 'hours', 'totalhours'] should be Trac defaults?

[Christopher Lenz]

Patch applied in [7499]. Thanks!

[Jeff Hammel <jhammel@…>]

Replying to osimons:

Patch seems to be committed to trunk in [7499], but I have some minor problems with it.

The patch conflicts with the efforts to provide e-mail obfuscation as fields like authorinfo(ticket.reporter) (and owner) that did obfuscation depending on permissions are now links that naturally must link to the true value to be meaningful.

I hadn't noticed this before and will look into it now.

At the very least, EMAIL_VIEW permissions should be checked before rendering links for owner and reporter. Additionally 'owner' and 'reporter' could be default for fields to not link-ify.

Or not link-ify if they are email-addresses. I have a hunch these should be rendered fields which don't get auto-link-ified, but am not sure without further investigation.

On the subject of defaults, I don't quite see why ['estimatedhours', 'hours', 'totalhours'] should be Trac defaults?

These are fields used by the TimingAndEstimationPlugin which are marked-up using JS. Now that the patch is in trac-core, hopefully the plugin authors can be persuaded to not do things this way. I will ticket the plugin soon, after investigation

[osimons]

Reopening for now until remaining issues reach some conclusion.

[Jeff Hammel <jhammel@…>]

Replying to osimons:

Reopening for now until remaining issues reach some conclusion.

Found the problem with osimons' help (haven't yet fixed yet):

• ​http://trac.edgewall.org/attachment/ticket/7562/autoquery_patch_vs_r7349.txt#L70

• ​http://trac.edgewall.org/attachment/ticket/7562/autoquery_patch_vs_r7349.txt#L73

I remove the call to authorinfo; this needs to be put back in for anonymizing

[osimons]

While testing, just adding some other nits discovered:

• It renders link for all fields before alternative rendering is applied. That means a checkbox that renders as 'yes' and 'no' does not get a link as that custom rendering replaces the content later in the code. It should link to 0 and 1, and keep the text it gets from whatever translation is used by the user.
• cc~=$USER is part of default_anonymous_query (although not working correctly), but as jhammel correctly points out, it does not make sense when the whole point of this setting is that the user is unknown (anonymous). It should be removed from default setting in query module.
• It linkifies any field, including custom fields of type text and textarea. For most use cases that will not be a useful behaviour. Also, see #1791 for the future of linking in text and textareas.

[Jeff Hammel <jhammel@…>]

patch to limit linking only to known users vs @7499

[Jeff Hammel <jhammel@…>]

patch to eliminate linking to the owner and reporter of tickets vs r7499

[Jeff Hammel <jhammel@…>]

Replying to Jeff Hammel <jhammel@…>:

Replying to osimons:

Reopening for now until remaining issues reach some conclusion.

Found the problem with osimons' help (haven't yet fixed yet):

• ​http://trac.edgewall.org/attachment/ticket/7562/autoquery_patch_vs_r7349.txt#L70

• ​http://trac.edgewall.org/attachment/ticket/7562/autoquery_patch_vs_r7349.txt#L73

I remove the call to authorinfo; this needs to be put back in for anonymizing

I have included two patches that solve this problem in different ways:

• attachment:autoquery_patch_linkknownusers_r7499.txt​ adds links in for the reporter and owner of a ticket only if they are known trac users

• attachment:autoquery_patch_dontlinkownerreporter_r7499.txt​ doesn't add these links in at all

Anther possibility is to have _query_link just return the url that is linked to and give this as an optional parameter to authorinfo which will markup the link if the reporter or owner is not anonymous. I haven't coded this yet, but if there is interest, I will

The easiest solution would be a check something like

<py:choose test="ticket['reporter'] == authorinfo(ticket['reporter'])">
<py:when test="True">${reporter_link}</py:when>
<py:otherwise>${authorinfo(ticket['reporter'])}</py:otherwise>
</py:choose>

However, since authorinfo lives in a macro and not in python-land, this won't work. I'd prefer if authorinfo lived in python-land, but don't understand why it doesn't and this may involve changing a fair amount (don't know, haven't tried it)

[Jeff Hammel <jhammel@…>]

improved version of attachment:autoquery_patch_linkknownusers_r7499.txt​ that uses Chrome.format_author for checking changes

[Jeff Hammel <jhammel@…>]

Replying to Jeff Hammel <jhammel@…>:

• attachment:autoquery_patch_linkknownusers_r7499.txt​ adds links in for the reporter and owner of a ticket only if they are known trac users

With osimons' suggestion to use format_author as a test, I wrote a better version of this patch that should have the correct logic: attachment:autoquery_patch_dontlinkobfuscated_r7499.txt​

This is my recommended solution, FWIW

[osimons]

Same patch, just a little less verbose.

[osimons]

Replying to Jeff Hammel <jhammel@…>:

This is my recommended solution, FWIW

Yeah, that seems to handle the obfuscation issue nicely. I attached a slightly less verbose version of the patch.

[Jeff Hammel <jhammel@…>]

Replying to osimons:

Replying to Jeff Hammel <jhammel@…>:

This is my recommended solution, FWIW

Yeah, that seems to handle the obfuscation issue nicely. I attached a slightly less verbose version of the patch.

looks good to me

[Jeff Hammel <jhammel@…>]

Replying to osimons:

While testing, just adding some other nits discovered:

• It renders link for all fields before alternative rendering is applied. That means a checkbox that renders as 'yes' and 'no' does not get a link as that custom rendering replaces the content later in the code. It should link to 0 and 1, and keep the text it gets from whatever translation is used by the user.

I hadn't thought about non-text custom fields. I'll look at this next.

• cc~=$USER is part of default_anonymous_query (although not working correctly), but as jhammel correctly points out, it does not make sense when the whole point of this setting is that the user is unknown (anonymous). It should be removed from default setting in query module.

+1; FWIW, the cc~=$USER doesn't seem to hurt anything that this patch does (that is, the query results are correct), but I'd like to see it gone. Another approach would be to have a separate Option for default_auto_query (or what not) that is separate, but I'd prefer to keep the number of options to the minimum.

• It linkifies any field, including custom fields of type text and textarea. For most use cases that will not be a useful behaviour. Also, see #1791 for the future of linking in text and textareas.

IMHO, textarea fields should not be linkified; I can see custom text fields as being useful to link to sometimes. Not sure if programmatic distinction can be made between potentially useful custom fields and non-useful custom fields as far as linking. Another option is to use the existing unlinked_fields option to specify non-useful fields. Or, the reverse — this Option could become linked_fields which by default contains the trac core fields, then other fields aren't linked unless they are added. What do people think of this?

[Remy Blank]

I agree that textarea fields should never be linkified, and I would also tend to say that text fields should not be linkified either, except for "reporter" and "owner". So +1 for a linked_fields option with a default of "all core non-text fields plus reporter and owner".

Another issue, checkbox fields are currently not linkified. Any reason why this should not be done, or is this just an oversight (there's a special case for _type == 'checkbox' at the bottom of _prepare_fields() that overrides the linkification)?

[osimons]

Well… Both linked and unlinked has its pros and cons for text fields. Textarea excluded of course, but perhaps also having non-core text fields off by default is the least obtrusive.

The issue with checkbox was noted in comment:7, and yes, those should be linked of course.

[Jeff Hammel <jhammel@…>]

Replying to rblank:

I agree that textarea fields should never be linkified, and I would also tend to say that text fields should not be linkified either, except for "reporter" and "owner". So +1 for a linked_fields option with a default of "all core non-text fields plus reporter and owner".

Another issue, checkbox fields are currently not linkified. Any reason why this should not be done, or is this just an oversight (there's a special case for _type == 'checkbox' at the bottom of _prepare_fields() that overrides the linkification)?

The reason checkbox fields aren't linked is just oversight — the links are put into a field's rendered attribute at the top of _prepare_fields with the idea that anything that overrides rendered should be used instead. With checkboxes, this is not the desired behavior and additional linking code should be done (around ​http://trac.edgewall.org/browser/trunk/trac/ticket/web_ui.py#L1151).

My proposal for an additional patch:

• fix the checkbox linking — they should link

• never link text areas

• change unlinked_fields to linked_fields with the defaults given in comment:13; maybe this should only apply to text fields (though linked_text_fields is a bit longish for an option name, though of course the behavior could be noted in the Option description).

How does this sound? If there is no huge controversy, I'd like to upload a patch early next week.

[Remy Blank]

Replying to Jeff Hammel <jhammel@…>:

How does this sound? If there is no huge controversy, I'd like to upload a patch early next week.

Sounds great! I would keep it as linked_fields, so you can disable linking for any type of field (if you wish).

[Jeff Hammel <jhammel@…>]

link checkboxes and don't link textareas

[Jeff Hammel <jhammel@…>]

i added a patch that fixes the checkbox (link these) and textarea (don't link these) behavior: attachment:autoquery_patch_r7508.diff​ (this also includes attachment:autoquery_patch_dontlinkobfuscated_r7499b.diff​); for this patch, i've kept it unlinked_fields though my next task is to write a patch that switches this to linked fields

[Jeff Hammel <jhammel@…>]

use linked_fields instead of unlinked_fields

[Jeff Hammel <jhammel@…>]

i've made a patch that uses a list of fields to link to (linked_fields) versus the previous fields not to link to (unlinked_fields): attachment:autoquery_patch_linked_fields_r7508.diff​

though now that i've done this, i think this is the wrong approach, perhaps. Currently, the patch is pretty dumb — it doesn't do anything based on the type of field (except it won't link textareas). so if you add a new custom field of any type, it won't be linked to unless you explicitly add it. One alternative is to make linking smarter — link all fields except text fields in all cases, only link text fields if they are listed in linked_fields. This way, adding a new text field never links whereas adding a select, checkbox, or radio button would automatically link. However, this gives no mechanism to not link select, checkbox, or radio button fields. Maybe this isn't a big deal (I know I wouldn't care if I couldn't unlink these fields, but don't know if this feeling is universal).

The alternative is just to go with unlinked fields — link everything by default except textareas. The drawback to this approach is if a new custom text field is added, it will be linked to until added to unlinked_fields. [One alternative to this is just not to link any text fields except owner and reporter].

I suppose my feelings aren't dramatically strong between these two options. I suppose my gut reaction would be to link everything except text fields unless they are explicitly mentioned in linked_fields.

In any case, once this decision is made, maybe a final patch is made and applied, if any cruft remains regarding ​th:TimingAndEstimationPlugin (hours, estimatedhours, totalhours) it should be removed and any associated issue ticketed with that plugin (as has been correctly pointed out, stuff in trac core should not refer to outside plugins).

[Jeff Hammel <jhammel@…>]

Replying to rblank:

Replying to Jeff Hammel <jhammel@…>:

How does this sound? If there is no huge controversy, I'd like to upload a patch early next week.

Sounds great! I would keep it as linked_fields, so you can disable linking for any type of field (if you wish).

Not sure if this is a typo; did you mean "I would keep it as unlinked_fields"?

[Remy Blank]

Replying to Jeff Hammel <jhammel@…>:

Not sure if this is a typo; did you mean "I would keep it as unlinked_fields"?

No, I did mean linked_fields, as opposed to your proposed linked_text_fields in the comment I replied to. But I have no strong opinion about linked_fields / unlinked_fields. I just feel that it will be easier to understand if the option applies to all field types.

[osimons]

Personally, unlinked_fields is my favorite - by default it just works for all fields (except textarea), and everything works the same way. If anyone for a special field or other reason should have a problem with that, then they can go and unlink what they don't want.

[osimons]

Cumulated tweaks + word linking.

[osimons]

Attachment t7562-autoquery_tweaks_r7548.diff​ provides a well-working solution, I think:

• It incorporates previous fixes for obfuscation and checkbox/radio rendering.
• It add word linking to keywords and cc fields, with 'contains' link for each word. Obfuscation for cc is handled in a very simple manner, so that if any content inside cc gets obfuscated then no links will render.
• It does not link to text and textarea fields, leaving that to perhaps some future .format=query option for rendering of custom fields - see discussion on #1791.
• As all default fields is handled + remaining custom fields should all be choice fields, there is no longer a need for a specific option to unlink/link fields. Option removed.
• The default query is also modified. The $USER is removed as it makes no sense for an anonymous query. Also, the ! in status is switched from != to =! as the GET parser does not comprehend the former (but both work as values for the setting).

Feedback please. Getting ready to commit…

[osimons]

Oops. Missed one unlinked reference. Use this cumulative patch, please - ignore previous.

[osimons]

Oh dear. These changes wreaks havoc with the tests: 825 tests. FAILED (failures=30, errors=4)… Anyway, test the user-side of things for now while I look into the tests :-(

[Remy Blank]

I haven't yet tested the patch, but I already have a question about the splitting algorithm in _query_link_words(). IIUC, you split at whitespace, strip ".,;" from both ends of every word, and combine again with a single space. This will change the apparent content of the fields in the display.

What about splitting with something like (untested):

for (i, word) in enumerate(re.split(r'(\s*[ .,;]\s*)')):
    if i % 2:
        # Generate tag here
        items.append(tag.a(...))
    else:
        items.append(word)

This will split at any of " .,;" and enclosing whitespace, and it will keep the separators and insert them in the output list, so it will keep the visual content of the field as it was before linkification.

[Remy Blank]

Oops, that should be:

for (i, word) in enumerate(re.split(r'(\s*[ .,;]\s*)')):
    if i % 2:
        items.append(word)
    else:
        # Generate tag here
        items.append(tag.a(...))

[osimons]

Updated cumulative patch without test failures (previous tried to split None type)

[osimons]

Replying to rblank:

This will split at any of " .,;" and enclosing whitespace, and it will keep the separators and insert them in the output list, so it will keep the visual content of the field as it was before linkification.

Tried latest variation, but not ideal as it will also split inside words so that individual emails also gets split for each dot(.). The dot is not really a divider, but i wanted to strip it from the end of words as this makes less sense for query (should perhaps use rstrip() to catch .net and similar?)

But, you are right - the split/strip algorithm can no doubt be improved.

[osimons]

Hmm. Having looked at it some more, I think that for such a rendering feature of 'keywords' and 'cc' what looks best overall is just having a uniform separator like comma:

...
        for word in value.split():
            word = word.rstrip(",.;")
            if word:
                link = req.href('query', **{name: '~' + word}) \
                            + (default and '&' + default or '') 
                items.extend([tag.a(word, href=link), ', '])
        return items and tag(items[:-1]) # Strip last comma

[Remy Blank]

Replying to osimons:

Tried latest variation, but not ideal as it will also split inside words so that individual emails also gets split for each dot(.). The dot is not really a divider, but i wanted to strip it from the end of words as this makes less sense for query (should perhaps use rstrip() to catch .net and similar?)

You're right, splitting on dots is no good. No idea why I put it in, actually.

What I find important is that the text displayed is the actual text in the DB, just linkified. I don't like the idea of "reformatting" values. This will lead to strange situations like having a field containing "Hello world", which will be displayed as "Hello, world", but where an "is" query for "Hello, world" will not return any results.

So I would still argue for splitting with a regexp of '\s*[ ,;]\s*' (i.e. without the dot).

[Remy Blank]

I have tested your latest patch, and the following additional changes make the output nice and clean:

trac/ticket/web_ui.py

@@ -1074 +1074 @@
             return value
         default = self.env.config.get('query', 'default_anonymous_query', '')
         items = []
-        for word in value.split():
-            word = word.strip(",.;")
-            if word:
+        for (i, word) in enumerate(re.split(r'(\s*(?:\s|[,;])\s*)', value)):
+            if i % 2:
+                items.append(word)
+            elif word:
                 link = req.href('query', **{name: '~' + word}) \
                             + (default and '&' + default or '')
-                items.append((tag.a(word, href=link), ' '))
+                items.append(tag.a(word, href=link))
         return tag(items)
 
     def _prepare_fields(self, req, ticket):

I have another concern. The query link generation method (href('query') with the specific field, and appending the default query) is fragile:

• It build a query URL by using query syntax. Is query syntax supported directly in query URLs?
• What happens if the default query already has a parameter for the specific field?
• Once #5998 will be applied, the default query options will allow both query syntax and URL query strings (in the same way as what the query: TracLinks support), so this will break.

A solution might be to decode the default query using a Query instance (using either Query() or Query.from_string() depending on the default query syntax), to get its constraints, update its constraints and get the link from Query.get_href().

[Christian Boos]

Replying to rblank:

A solution might be to decode the default query using a Query instance …

Note that Query instances are not cheap due to the #6436 bug.

[osimons]

Replying to rblank:

I have tested your latest patch, and the following additional changes make the output nice and clean:

Thanks a lot - that looks like a keeper :-)

As for your remaining query questions, I'm quite ignorant as I've not spent much time inside the query module code. I've just reused what this ticket had already implemented in an effort to make this new feature work better. I do notice strange query behavior like difference between != and =! that I can't quite explain, but that should be more improvements to the query module rather than side issues to this ticket. I'm OK with constructing simple href based links and fixing it later should it prove to be a problem.

[Remy Blank]

Replying to osimons:

I do notice strange query behavior like difference between != and =! that I can't quite explain, but that should be more improvements to the query module rather than side issues to this ticket.

According to TracQuery#UsingTracLinks, there are two syntaxes for ticket queries:

• The query language, which is parsed by Query.from_string() and uses the syntax where the modifier is before then =, like ~=.

• URL query strings, which are the part following /query in the custom query URL. They start with a ? and place the modifier after the =, like =~. They are processed minimally in Query.process_request() and passed directly to Query.__init__().

When constructing an href URL, you should always use the latter. However, the default_query options contain a query in the former syntax (at least until #5998 is solved). That's why you are seeing strange results: the generated URL is actually wrong. You can see how query: TracLinks are generated in `_format_link()`​. The same should be done here.

But as Christian mentions, constructing the Query object is currently expensive (and this will probably need FieldRefactoring to be fixed). What you could do is to have a separate autoquery_query option instead of using default_query, and require that option to be in URL syntax. It would then be relatively cheap to parse with the parse_query_string() function introduced in #5998. If you want, I can do that after closing that ticket.

[osimons]

Replying to rblank:

According to TracQuery#UsingTracLinks, there are two syntaxes for ticket queries:

Thanks for explaining. I'll make a ticketlink_query option to keep these different, and just read and use that directly for now.

[Remy Blank]

#5998 has been closed in [7553]. This gives you a function parse_query_string() that takes a query string and returns a _RequestArgs dictionary containing the query arguments. You should be able to use that to parse the ticketlink_query option, then override the autoqueried field and finally use href to construct the link.

[osimons]

Cumulative patch using new parse_query_string.

[osimons]

Updated patch with a few additions:

• A new [query] ticketlink_query = option
• Using new parse_query_string()
• Minor changes to parse_query_string to a) discard starting ?, and b) better handle empty query string.

Thanks for input everyone. I think the patch should be good enough now. Testing is most welcome, but suggestions for additional features or reworked implementation is not… ;-)

[Remy Blank]

Replying to osimons:

• Minor changes to parse_query_string to a) discard starting ?, and b) better handle empty query string.

Looks very good!

I have a minor nitpick, though. At some point, when #6436 will be solved, we'll be able to also support query language in [query] ticketlink_query, in the same way as default_query, and we'll differentiate between both syntaxes by the leading ?. So I would strip the ? in _query_link() and _query_link_words() instead of parse_query_string().

And BTW, you don't need a replace() for that, just do a query_string[1:] :-)

[osimons]

Replying to rblank:

So I would strip the ? in _query_link() and _query_link_words() instead of parse_query_string().

Oki.

And BTW, you don't need a replace() for that, just do a query_string[1:] :-)

I use .replace() in order to use string methods instead of slicing an array, as I seem to remember seeing and fixing some issues at some stage whereby some Asian two-byte charsets evaluated this differently. I'd much rather do it the easy way, and in fact I had that as well before I got thinking. Is [1:] always safe? That would be good to know.

[osimons]

I suppose it is just me complicating strings much more than necessary - at the point where we evaluate it all should be Unicode anyway… Oki, I'll fix last minor details and get this committed.

[osimons]

Committed in [7560]. Closing.