[PATCH] Add support for configuring a default set of columns to show in a query.

[Torbjörn Svensson <azoff@…>]

At my work, we are using trac to view our bugs and tasks, but one of our customer uses JIRA, so we export our issues from JIRA and import them in trac as a tickets. To keep track of the issues, we have a custom field called 'jira' that contains the upstream id. As the upstream id is quite important, we want to include the column in the set of default columns for a query. The attached patch introduces two new config directives, one for setting the default order in the query (rather than hardcoding it to be 'priority', still a fallback through…), the second is a list of columns to automatically show unless the user specified a set of columns.

Examples: Using this link results in the following set of columns shown in the browser (verified here on t.e.o):

• Ticket
• Summary
• Owner
• Type
• Status
• Priority
• Milestone

With my patched applied and the following in the trac.ini

[query]
default_cols = id,time,changetime,priority

would result in the following columns shown in the browser:

• Ticket
• Created
• Modified
• Priority

[Remy Blank]

Doesn't the [ticket] default_query do exactly what you want? You can specify the columns to show, provided you use the URL syntax.

[osimons]

This has been a topic on IRC recently, and you are right in that the default_anonymous_query and default_query options work when using URL syntax like ?version=1.0&col=summary&col=milestone&order=id&desc=1.

What the submitted patch achieves though, is setting default columns and order when not specified - for instance a request for /query?milestone=0.12.3. This can be links generated by Trac (like on the roadmap), or links made using query: linking. The columns that are default displayed cannot currently be modified - removing unwanted fields, and perhaps adding important custom fields like the example in description. Currently we just collect columns and discard all but the first 7.

What I'm missing with the default_order though is the ability to mark order as desc like my URL example above. Perhaps make desc an optional add-on so that we can do default_order = id desc.

[Remy Blank]

Replying to osimons:

What the submitted patch achieves though, is setting default columns and order when not specified - for instance a request for /query?milestone=0.12.3. This can be links generated by Trac (like on the roadmap), or links made using query: linking.

Ah, I see. Thanks for clarifying. The columns can be specified in query: links, but I see that it's more convenient if you don't have to. Being able to specify the descending column would be nice indeed.

(Scheduling for next-major until we have a reasonably complete patch.)

[Torbjörn Svensson <azoff@…>]

Replying to rblank:

(Scheduling for next-major until we have a reasonably complete patch.)

I will look into the desc/asc request as soon as possible, but it will probably have to wait to Monday through. Would you like a separate option for this, or do you want to have it included in the default_order option somehow and parsed manually in the code? Or do you have a better solution?

[osimons]

Oh, and for feature completeness I suppose there could also be a default_group option. Likely rarely used and default empty.

[osimons]

Replying to Torbjörn Svensson <azoff@…>:

Would you like a separate option for this, or do you want to have it included in the default_order option somehow and parsed manually in the code?

As mentioned, my preference would be to support desc (and asc) after field names and just parse it. Match for case-insensitive desc is descending, all other is just asc as usual. This will work at least as long as we only support ordering by one field at a time:

[query]
default_order = changetime desc

[Torbjörn Svensson <azoff@…>]

Replying to osimons:

Oh, and for feature completeness I suppose there could also be a default_group option. Likely rarely used and default empty.

Actually, I started implementing this, but ended up throwing it away as I was unable to make a query request without grouping. It may be related to how I implemented it, but from what I found out, the only way to view a query without grouping was to use group=None in the query call. So, how would we distinguish a non-grouping request from a request without grouping defined?

[Torbjörn Svensson <azoff@…>]

Replying to osimons:

As mentioned, my preference would be to support desc (and asc) after field names and just parse it. Match for case-insensitive desc is descending, all other is just asc as usual.

Roger that. I'll prepare a patch for this as soon as I got an hour to spare.

As a side question, should I split this enhancement into more than one patch? Like, one for default_order, another for default_cols and a possible third for default_group?

[osimons]

Replying to Torbjörn Svensson <azoff@…>:

Roger that. I'll prepare a patch for this as soon as I got an hour to spare.

Nice. Thanks.

As a side question, should I split this enhancement into more than one patch? Like, one for default_order, another for default_cols and a possible third for default_group?

No, don't split the patch. It belongs together. A new updated version of the patch is preferable to me.

[osimons]

Also note #10178 that suggests making it possible to order by multiple fields, and that very likely will also adjust the syntax for specifying order.

[Torbjörn Svensson <azoff@…>]

I've finally got the time to work on this issue again. This patch adds support for default cols, default order and default group. There are however one items remaining in order to have complete support for ordering by multiply fields, how would the user define the order? In the code, there are three reference to #10178, AFAIK, those are the only two places in the code that needs to be updated to have complete support for multi order.

One of my main objectives with this patch was to keep backward compatibility, so the old

order=foo&desc=1

format should still work.

I've also taken the liberty to use the '-'-notation to indicate reverse ordering in the config so the config directives use the same syntax as the URL and macro. Hope it's okay.

Comments on the patch is much appreciated. This is almost my first work in python, so there's probably many statements that could be reduced.

[Torbjörn Svensson <azoff@…>]

Patch adding default_cols, default_group and default_order. Also adds some initial support for multi order (see #10178).

[osimons]

Thanks for picking it up again. I've done some reading and testing, and it holds great promise. Here are my notes:

• /query?order=id&desc=1 does not actually work. It sorts ascending regardless of what I try.
• Why change from order to orders? As in default_orders too? Can we not keep order only?
• As the Query internal APIs change, stable 0.12 is not likely the correct target for the patch. It will likely need to be rebased on 0.13 at some stage.
• The OrderRule class seems somewhat like overkill for what it actually does (keeping the order field + descending marker together). Could not a dict be used instead so that it for instance looks something like:

  self.order = [{'name': 'milestone', 'desc': True}, {'name': 'id', 'desc': False}]
  

  Same goes for using the order list in use; we should in tests and more use the string representation using the new syntax. Query.from_string() should continue to support order='id'. The dict or class should be kept internal if possible, and not be required knowledge for correct calling of the API.
• Should the default_cols options actually be called default_columns? We use col for user queries but not cols anywhere seen by the user. So, if different, would it not make sense to just spell it out in full? It reads better IMHO.

Other than that, the default columns seems to work fine - which after all was the original intention of this ticket.

[Torbjörn Svensson <azoff@…>]

Replying to osimons:

Thanks for picking it up again. I've done some reading and testing, and it holds great promise. Here are my notes:

• /query?order=id&desc=1 does not actually work. It sorts ascending regardless of what I try.

It should work, I'll take another look at this unless we could agree on drop the desc param. See my other comments below.

• Why change from order to orders? As in default_orders too? Can we not keep order only?

There was basically only one reason, to have it reflect that there may be more than one rule, hence make it analog with how cols is handled. If I'm not mistaken, the user will supply multiple occurrence of the col param in the URL and those are collected in a list called cols internally.

• As the Query internal APIs change, stable 0.12 is not likely the correct target for the patch. It will likely need to be rebased on 0.13 at some stage.

It's probably a good idea to move this to 0.13 as it more or less is bound to break some environments. I would also prefer if we could drop the old /query?order=id&desc=1 way do define a rule. Dropping it would make the implementation a lot cleaner and probably less buggy.

• The OrderRule class seems somewhat like overkill for what it actually does (keeping the order field + descending marker together). Could not a dict be used instead so that it for instance looks something like:

  self.order = [{'name': 'milestone', 'desc': True}, {'name': 'id', 'desc': False}]
  

  Same goes for using the order list in use; we should in tests and more use the string representation using the new syntax. Query.from_string() should continue to support order='id'. The dict or class should be kept internal if possible, and not be required knowledge for correct calling of the API.

Sure, we could use a dict… The drawback with a dict is that there are no easy way to implement a fallback default value and we need to handle the stringification on a few different places. My first go with this bug was to use a dict, but I later changed to a class to make the implementation a bit cleaner. Regarding the test cases, I tried to have both just to make sure that the implementation was correct. Unless there are one or more test cases using lists, some errors may be masked by using the Query.from_string() and that would make the test useless.

• Should the default_cols options actually be called default_columns? We use col for user queries but not cols anywhere seen by the user. So, if different, would it not make sense to just spell it out in full? It reads better IMHO.

I just kept the name so close to the real name as possible, but yes, default_columns sounds a lot better.

Other than that, the default columns seems to work fine - which after all was the original intention of this ticket.

Good to hear, may you add a comment on #10178 too regarding how to present the sort order to the user? To sort by a single field (as this bug had in mind) would be identical to how it is currently implemented in HEAD, but with the addon of #10178, that will change.

[osimons]

Replying to Torbjörn Svensson <azoff@…>:

Just hold off on further changes until Remy or Christian offer their opinion. Particularly rblank has had a heavy hand on the Query module in recent years.

It would be useful to get more guidance on where to take this patch in order to get it ready for commit.

[Remy Blank]

I haven't had time to really review the patch, and won't have before my holidays next week, but let me comment on what I got from reading the comments and browsing the code:

• If at all possible, I would prefer keeping the Query() interface backward-compatible. For example, can we keep the current order argument to the constructor, and have an additional argument for the new functionality?
• The patch is huge, and I have a hard time understanding why this should be so. Your initial patch was more in the expected size range, I would even aim for less than that. But maybe I'm missing something about the complexity of the desired functionality.
• The __getattribute__() method is one of the more "magical" methods of Python objects, a bit like metaclasses, and therefore raises a red flag. Actually, most of the OrderRule class, if not all, is unnecessary. For example, we tend to not check argument types, and we don't define __str__(), __repr__(), __eq__() or __ne__() if we don't use them for a specific purpose.

As I said, this is only based on skimming the patch, and I may have missed a lot of the details. I'm not sure I can find the bandwidth in the next few weeks to review such a big patch, unfortunately.

[Remy Blank]

Re-reading comment:16, this sounded harsh and wasn't intended to. I really need to find time first to dive into the code.

Considering the size of the patch, it would probably make sense to split the functionality into several patches, adding one step at a time. Maybe start with a single-column configurable default sort order (and specifying descending as e.g. "-priority"), have it reviewed and applied. Then, add configurable default columns, review, apply. And finally add multi-column ordering, maybe as a list of strings (e.g. ["priority", "-created"]).

This would greatly simplify the review, and would allow small refactorings as necessary, instead of one big refactoring.

[osimons]

(Note:) Just like order & desc there is also group & groupdesc currently supported - so for instance groupdesc=1&group=milestone will provide a grouped milestone query that sorts groups descending.

[Torbjörn Svensson <azoff@…>]

Replying to rblank:

I haven't had time to really review the patch, and won't have before my holidays next week, but let me comment on what I got from reading the comments and browsing the code:

• If at all possible, I would prefer keeping the Query() interface backward-compatible. For example, can we keep the current order argument to the constructor, and have an additional argument for the new functionality?

Sure, I'll try to implement this.

• The patch is huge, and I have a hard time understanding why this should be so. Your initial patch was more in the expected size range, I would even aim for less than that. But maybe I'm missing something about the complexity of the desired functionality.

Yes, it became quite huge… However, I do not think it's possible to shrink the code size to a desired size and still have the functionality needed.

• The __getattribute__() method is one of the more "magical" methods of Python objects, a bit like metaclasses, and therefore raises a red flag. Actually, most of the OrderRule class, if not all, is unnecessary. For example, we tend to not check argument types, and we don't define __str__(), __repr__(), __eq__() or __ne__() if we don't use them for a specific purpose.

Right or wrong… but I actually use the __getattribute__() magic to have a fallback value to use if the rule is invalid. A rule can be invalid due to that the order and desc params were separated in previous versions. The purpose of the OrderRule class was to provide a way to strinigify and destringify without needing to have the logic in more than one place. As a consequence, it provides a way to keep other parts of the code simple and clean even as the operations performed may not be trivial.

As I said, this is only based on skimming the patch, and I may have missed a lot of the details. I'm not sure I can find the bandwidth in the next few weeks to review such a big patch, unfortunately.

I'll be happy to split the patch into smaller sections. It's somewhat logical to dived the patch into one for default order, a second for default columns and a third for default grouping. I actually asked if I should do this, but osimons turned this question down in comment:10.

Replying to rblank:

Re-reading comment:16, this sounded harsh and wasn't intended to. I really need to find time first to dive into the code.

Considering the size of the patch, it would probably make sense to split the functionality into several patches, adding one step at a time. Maybe start with a single-column configurable default sort order (and specifying descending as e.g. "-priority"), have it reviewed and applied. Then, add configurable default columns, review, apply. And finally add multi-column ordering, maybe as a list of strings (e.g. ["priority", "-created"]). This would greatly simplify the review, and would allow small refactorings as necessary, instead of one big refactoring.

Works for me, I'll be happy to produce a patch series instead. However, I would like to have each patch commited before creating the next one or I will have to somehow mirror the current tree and dived the patches using it (more work…). Each of the patches should be pretty small, so it wouldn't be too much trouble for you.

[osimons]

I don't think I foresaw the complexity and patch size at the time… Sorry about that.

Anyway, do you have a BitBucket user? If so we could (q)clone Trac repos with a patch queue and just start at the items in order and build the stack of patches for trunk / 0.13dev?

[osimons]

I've created a BitBucket patch queue for this, see: ​https://bitbucket.org/osimons/trac-t10425/src

To get things going I've extracted the default_columns part of the big patch, and added it as the first patch of the series. It is a nice self-contained piece of code that just replaces the hard-coded selection of columns. The next would be to start attacking the remaining parts of the patch, adding each feature in its own patch. Each building on the previous.

How does Mercurial Queues at Bitbucket work? To let everyone in on the secrets, I have written a blog post with details of the why, what and how:

​https://www.coderesort.com/u/simon/blog/2011/12/17/bitbucket_patch_queues

[Torbjörn Svensson <azoff@…>]

Finally, I've managed to create a full patch series for this ticket with a lot of help from osimons. The resulting patches can be viewed in osimons bitbucket repo at ​https://bitbucket.org/osimons/trac-t10425/src.

---

The following three patches can be considered 0.12-material due to their simplicity:

• The default_columns.diff introduces the config directive to define a default list of columns.
• The default_order.diff introduces the config directive to define a single default field to order the queries by.
• The default_group.diff introduces the config directive to define a default filed to group tickets by.

---

The following patch is to be considered 0.13-material only as it makes quite some changes to the UI(not completed yet, see comments in code and below) and are thus bound to create some trouble. It should be backward compatible, but there may still be bugs to iron out.

• The new_format3.diff takes the three default_*.diff patches and joins them with ticket #10178.

Due to the nature of how the pairs order/desc and group/groupdesc worked in the past and the need to define multiple order directives, it was discussed and agreed that a new format was needed. The new format looks like this:

• foo or +foo sorts ascending
• -foo sorts descending

This new format works for both the group and the order parameter to the constructor for Query. It also works for the TicketQuery-macro. It does even work in the UI as long as you only use a single field to order by. If you use two or more, you will only see the first one and you will only be able to do this by directly modifying the URL or using the macro.

---

All of the patches introduces unit tests covering as much as possible, there are currently a few of the tests that fails. These failures needs to be debated a bit more.

1. What should happen if the user supplied group is invalid? Should the configured default group be used and possibly validated too? Or should the query just be non-grouped?
2. What should happen if the user supplied order is invalid? Should the configured default order be used and possibly validated too? Or should the query just be ordered by 'priority' as it were before?

I and osimons have talked a bit about them but haven't been able to agree on a good way to handle them. What are your opinion?

[osimons]

Replying to Torbjörn Svensson <azoff@…>:

All of the patches introduces unit tests covering as much as possible, there are currently a few of the tests that fails. These failures needs to be debated a bit more.

I'll add my opinion on these here - to breathe life into the discussion:

1. What should happen if the user supplied group is invalid? Should the configured default group be used and possibly validated too? Or should the query just be non-grouped?

My take on the defaults is that, like most other parts elsewhere in Trac, we call upon them only when nothing else is passed. We don't call on them if the specified group or order is invalid. IMHO order=mielstone (typo) should not order by priority. As the names are passed to the SQL in various clauses, the names must also be verified for all cases. That is why I think that we should simply do (pseudo) order = args.get('order') or default_order and use it if valid name(s). Unexpected output for errors is quite OK - and this is just a query after all.

2. What should happen if the user supplied order is invalid? Should the configured default order be used and possibly validated too? Or should the query just be ordered by 'priority' as it were before?

See 1 for part of that answer. As for order, we also always append the ticket id to order. To me even that seems sufficient for the cases where the supplied or configured order is invalid.

---

There is one more issue about the patch that we have discussed and don't quite agree: I would like query.get_group() to just return the name of the group and not a (group, desc) tuple. We only ever use it once in a template and having to do query.get_group()[0] all over the place is just ugly. The one instance where we may need it, (group, desc) will be the same as query.get_order()[0] seeing we now insert group information as part of internal order.

[shesek]

If anyone is interested, I wrote a small plugin that allows setting that option: ​https://gist.github.com/1923150

Its quite hacky (modifies the col query string argument on pre_process_request), but it works.

[figaro]

Plugin known under ​th:DefaultColsPlugin