Extend TicketQuery macro to render reports

[Ryan J Ollos]

TicketQuery macro could be extending to provide the equivalent of ​th:WikiReportMacro, rendering reports and saved queries by id.

[walty <walty8@…>]

I suppose I should add a new parameter (e.g. report_id) for the TicketQuery module, is that correct?

btw, should I build the logic from scratch, or I better start from the code inside the WikiReportMacro?

thanks

[Jun Omae]

I think we should add new macro for reports rather than extend TicketQuery macro. The several parameters of TicketQuery macro are not available for reports, e.g. format, group, verbose, col and reports can use variables, e.g. USER.

[Ryan J Ollos]

I agree with adding a ReportMacro rather than extending TicketQuery.

[walty <walty8@…>]

But if ​th:WikiReportMacro already exists, does that mean there is actually no further thing that need to be done?

[Ryan J Ollos]

I think it makes sense to add the equivalent to Trac.

[walty8@…]

hi,

Here is a quick patch regarding to the issue: ​de62ac71.diff.

(​de62ac71 for a view-friendly page)

Please note that, I just shameless copied the code from the ​​th:WikiReportMacro.

Since the WikiReport.html in the plugin has overlapped with report_view.html in trac core, I did a simple refactoring of report_view.html, and move the common part into a new template file (report_table.html). I am not sure if this is appropriate.

And two quick questions here:

1. This time I handed in the patch as github diff link instead of patch file, I wonder if this is fine?

2. How should I write unit tests for this one? Any direction please?

Thanks.

[Jun Omae]

Replying to walty8@…:

And two quick questions here:

1. This time I handed in the patch as github diff link instead of patch file, I wonder if this is fine?

Thanks. I'll review again. Well, could you kindly create a branch like t12171 rather than trunk in your repository?

2. How should I write unit tests for this one? Any direction please?

I think TicketQueryMacroTestCase and QUERY_TEST_CASES​ probably are of help to add unit tests.

[Ryan J Ollos]

For staging work on GitHub as a contributor without commit access, I learned from tips in comment:11:ticket:11028. Someday it would be nice to extend TracDev/SubmittingPatches with tips for providing changes via a fork on GitHub, so we can help potential contributors get up to speed quickly with the modern tools.

[walty8@…]

thanks ryan for the suggestion.

Here is the branch for this ticket:​1.0-stable-t12171

And here is the comparison link for the final patch: ​compare

In the latest commit, the followings are done:

1. Moved the macro from wiki/macros.py to ticket/query.py, since the code is more close to TicketQuery module.

2. Added unit tests

3. (As suggested by Jun Omae) Restored the req.args after the macro rendering, so as to remove the side effect to other modules.

[Ryan J Ollos]

Initial feedback: Probably ticket/report.py is the most appropriate location.

[anonymous]

Replying to Ryan J Ollos:

Initial feedback: Probably ticket/report.py is the most appropriate location.

Actually I thought about that, but in that case some of the unit test case logic (e.g. ticket_setup and ticket_tear_down) may not be reused easily. Is it OK if I import those functions from trac/tests/query.py into trac/tests/report.py?

[Ryan J Ollos]

Added some comments to ​e49f1cf8.

Also:

• You can ​squash your changes and then ​cherry-pick the single changeset over to a new branch based on the trunk, so that your changes are based on trunk (comment:9).
• The main test logic needed to create macro tests in trac/tests/report.py is provided by trac.wiki.tests.formatter. I imagine you may also want to use the method _insert_tickets. Code like that can be copied, but just make sure you understand the code you are copying. Eventually I'd like to find a way to eliminate some of the duplication (#12211).

[Ryan J Ollos]

Replying to walty8@…:

Here is a quick patch regarding to the issue: ​de62ac71.diff.

(​de62ac71 for a view-friendly page)

Please note that, I just shameless copied the code from the ​​th:WikiReportMacro.

I think it's a good start to copy code from WikiReportMacro, but it uses a hack of calling ReportModule._render_view and we'll want to implement a more proper solution. I think it may involved extracting some code from ReportModule to a function or method. See also #12219, which I will try to finish up soon. Changing ReportModule may be the most challenging aspect of this ticket, if you can make all the other changes first then we can tackle that at the end. In the meantime you can continue calling ReportModule._render_view, which needs a req object. Since we don't want to change the args of the req object that is passed in formatter.req, you can probably use copy.deepcopy to create a new Request object. If you have problem with that you could try to create a mock request with Mock from trac.test.

I'll try to get back to you more quickly in the future so we can iterate towards a solution.

[walty8@…]

Replying to Ryan J Ollos:

Added some comments to ​e49f1cf8.

Also:

• You can ​squash your changes and then ​cherry-pick the single changeset over to a new branch based on the trunk, so that your changes are based on trunk (comment:9).
• The main test logic needed to create macro tests in trac/tests/report.py is provided by trac.wiki.tests.formatter. I imagine you may also want to use the method _insert_tickets. Code like that can be copied, but just make sure you understand the code you are copying. Eventually I'd like to find a way to eliminate some of the duplication (#12211).

One quick question, if I squash the changes first, would it affect the code review comments on github?

[Ryan J Ollos]

Replying to walty8@…:

One quick question, if I squash the changes first, would it affect the code review comments on github?

Yes, it will. That's why you should never rebase a branch that you've made public. Instead, you create a new branch based on your branch and rebase your new branch, leaving your old branch frozen in time forever. Usually we just append a .1, .2, … as we create new branches that need to be rebased. For example, see #11901 in which I'm currently working with .15, but have not yet published (log:rjollos.git:t11901-remove-deprecated-code-in-1.3.1.14 in comment:37:ticket:11901 is the latest published).

$ git checkout -b t12171 1.0-stable-t12171
$ git rebase -i

For your case, since we even want to go one step further and have your changes based on trunk rather than 1.0-stable, we can do the following;

$ git log --oneline -5 1.0-stable-t12171
075b5a0 remove extra line due to false merge process of previous
e49f1cf moved macro to trac/ticket/query.py, added unit tests, and restored req.args after macro finished
24da572 uploaded missing file, and fixed code review comments by Jun Omae
38f1c97 cherry pick commits de62ac711e514f32bef89772106bdc8996cf6d7a and 85081cba5cb487f87bff23c9b4ebec33fbdba510
a516b71 1.0.10dev: Workaround case-sensitivity defect in setuptools
# Note that your changes are based on the tip of 1.0-stable (a516b71)
$ git checkout trunk
# You should have added the mirror as described in https://trac.edgewall.org/wiki/TracTeam/Repositories#git
# If you haven't added the mirror yet, run these next two steps. If you have, skip them.
$ git remote add mirror http://svn.edgewall.org/git/trac/mirror
$ git fetch mirror
# Pull latest changes from the mirror to trunk.
$ git pull mirror trunk
$ git log --oneline -1 
cf506ee 1.2dev: Unselect all other search filters when modifier key selected   #<-- this is the latest commit on trunk
$ git checkout -b t12171
$ git cherry-pick 1.0-stable-t12171~4..1.0-stable-t12171
error: could not apply 38f1c97... cherry pick commits de62ac711e514f32bef89772106bdc8996cf6d7a and 85081cba5cb487f87bff23c9b4ebec33fbdba510
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
$ # resolve conflict in trac/wiki/macros.py ...
$ git add trac/wiki/macros.py
$ git cherry-pick --continue
$ # repeat pattern until all conflicts resolved and all changes applied ...
$git status -sb
## t12171
$git log --oneline -5
5569691 remove extra line due to false merge process of previous
226e380 moved macro to trac/ticket/query.py, added unit tests, and restored req.args after macro finished
53bb63b uploaded missing file, and fixed code review comments by Jun Omae
adb427f cherry pick commits de62ac711e514f32bef89772106bdc8996cf6d7a and 85081cba5cb487f87bff23c9b4ebec33fbdba510
cf506ee 1.2dev: Unselect all other search filters when modifier key selected
# Note that your four changesets are based on the tip of the trunk (cf506ee)
# Now we are finally ready to rebase
$ git rebase -i

Actually, I wouldn't be surprised if there's a better way to do that. With Git there's always several ways to accomplish the same task.

Future rebases will be much simpler.

$ git checkout trunk
$ git pull mirror trunk
$ git checkout -b t12171.2 t12171
$ git rebase trunk
$ git rebase -i   # Squash, fixup, edit commit message, etc ...

[walty8@…]

Replying to Ryan J Ollos:

(snip)

Thank you very much for the detailed explanation! I finally managed to work out a new single commit on the new branch.

I would push the new commit to GitHub after all the previously raised comments are fixed.

_{(please just quote what you need, no more, thanks! For example, if you have to address several different points raised in one comment, it's good to keep an excerpt for replacing the reply in context; here the "comment:18 in reply to 17" is enough to get the context)}

[Christian Boos]

Replying to Ryan J Ollos:

$ git checkout -b t12171
$ git cherry-pick 1.0-stable-t12171~4..1.0-stable-t12171
...

Actually, I wouldn't be surprised if there's a better way to do that. With Git there's always several ways to accomplish the same task.

Yes, I think it's better to use the "rebase onto" way, instead of cherry-picking:

$ git checkout -b trunk-t12171 1.0-stable-t12171
$ git rebase 1.0-stable trunk-t12171 --onto trunk
# conflict resolution as before, except with `git rebase --continue/--abort` now

You could pass -i already at that step, and squash things in one go. Depending on the situation it might minimize the number of conflict resolution (say you have a conflict in first commit, subsequent commits keep modifying the lines you had to change in order to resolve the conflict → more conflicts).

You may name the branches as you want, but for ticket fixes, tXXX[.n] is a good choice, and if you have to add the base line (as soon as you have to disambiguate), I'd prefer to use the version as a suffix rather than a prefix (i.e. here, you could have started with just t12171, then t12171-trunk).

[Christian Boos]

And finally, now that you know how to rebase on arbitrary branches, why not be future-proof and have a try at rebasing your changes on the cboos.git@jinja2 branch? ;-)

(until we make the branches from the TracTeam/Repositories available again without the need to authenticate first, you can fetch the branch from its ​github mirror)

[Ryan J Ollos]

Thanks for the additional tips. I'll use them when revising TracDev/SubmittingPatches (comment:1:ticket:12358).

[walty8@…]

hi,

Sorry for the late response. here is my latest patch: ​8500a81.

Unfortunately I failed not accomplish a full refactoring of _render_view and self.execute_paginated_report, as it's just too hard for me. However, I added an optional argument args to the function _render_view, so as to avoid the copy of req.args.

Here are some uncertainties I would like to raise:

1. I imported an internal class named _RequestArgs, which is odd

2. I removed the public function of get_var_args in order to remove the dependency of req.args. I tried the grep and make sure no others used this function, but I am not sure if it’s used by other Trac plugins. If it’s necessary to keep this function, I would need to use some other work around which would add extra complexity to the small function.

3. In the unit tests, I copied the method of ticket_setup and ticket_teardown from trac/ticket/tests/query.py to trac/ticket/tests/reports.py

4. For some reasons, the squashed changed still appear in the github website, yet the commit mentioned above should be complete and latest.

[walty8@…]

Replying to Christian Boos:

And finally, now that you know how to rebase on arbitrary branches, why not be future-proof and have a try at rebasing your changes on the cboos.git@jinja2 branch? ;-)

I am sorry I did not fully understand the git rebase .. onto thing, I finally used the most basic way to accomplish it:

git checkout -b t12171.3.cboos
git rebase mirror.cboos
#a lot of git rebase --skip
git rebase -i HEAD~5
git push

The final commit is ​4493f81, and it seems to work.

Please see if this is fine, thanks.

[Christian Boos]

Replying to walty8@…:

Replying to Christian Boos:

And finally, now that you know how to rebase on arbitrary branches, why not be future-proof and have a try at rebasing your changes on the cboos.git@jinja2 branch? ;-)

I am sorry I did not fully understand the git rebase .. onto thing, I finally used the most basic way to accomplish it:

git checkout -b t12171.3.cboos
git rebase mirror.cboos
#a lot of git rebase --skip
git rebase -i HEAD~5
git push

Well, no, you missed something here, you just rebased on trunk somehow, not onto the Jinja2 branch.

The idea is that you to a rebase like that:

trunk t12171.3 | | ----------------++++ \--------- | jinja2

trunk t12171.3 | | ----------------++++ \---------**** | | jinja2 t12171.3.jinja2

The point of that branch is to go away from Genshi templates and use Jinja2 templates, so instead of a brand new Genshi template trac/ticket/templates/report_table.html, you need to refactor the trac/ticket/templates/jreport_view.html, extract the table rendering part into a separate Jinja2 template jreport_table.html, and then include it from trac/ticket/templates/jreport_view.html and also use it from the macro. The call to Chrome.render_template should remain the same.

[walty8@…]

Replying to walty8@…:

hi ryan, please use ​bdc051 for the final patch, I found that the previous merge process missed one refactoring in the trac/ticket/templates/report_view.html.

[Ryan J Ollos]

One of the committers will need to do some refactoring work in the report module to address the areas you didn't feel comfortable touching, so this work won't make it in until milestone:1.3.1. Given that, the work will need to be rebased on Christian's Jinja2 branch and the templates ported to Jinja. Next step is to follow his advice in comment:24 and see if you can get it rebased on his branch and working with a Jinja2 template.

[walty8@…]

hi Christian,

Please find the latest patch using jinja version in ​5c1ed9.

It's actually very easy to use the jinja2 templating system, but the git rebase or git cherrypick is an absolute pain! It took me 5-6 different attempts to do the merge.

The main problem I got is that I attempted to merge some commits that are already pushed. (This is because I have to use different machines to do development in different places, and I can't put all unpushed commits in one place and rebase them.) And that caused branch diverge and then all kinds of subsequent headaches.

Eventually I used the following approach to consolidate the changes.

E.g. I need to rebase last 4 commits of branch_a and consolidate them into a single commit in branch_b for each better patch submitting:

1. do a rebase for the remote commits (already pushed commits)

   git rebase -i origin/branch_a~4 origin/branch_a 
   

2. create a new branch using the detached HEAD (created from step 1)

   git branch branch_b HEAD
   

3. push the new branch to github

   git checkout branch_b
   git push --set-upstream origin branch_b
   

[Christian Boos]

Replying to walty8@…:

hi Christian,

Please find the latest patch using jinja version in ​5c1ed9.

Thanks! That one looks much easier to review, indeed.

It's actually very easy to use the jinja2 templating system, but the git rebase or git cherrypick is an absolute pain! It took me 5-6 different attempts to do the merge.

Also, the fact that you had to add getint and so on is simply because my branch is now lagging and needs to be rebased on latest trunk as well.

So roughly the plan is still 1.2 → 1.3.1 clean-ups from Ryan → (py3k changes from Jun)? → Jinja2 → (this ticket).

[Ryan J Ollos]

Replying to walty8@…:

hi Christian,

Please find the latest patch using jinja version in ​5c1ed9.

It's actually very easy to use the jinja2 templating system, but the git rebase or git cherrypick is an absolute pain! It took me 5-6 different attempts to do the merge.

The main problem I got is that I attempted to merge some commits that are already pushed. (This is because I have to use different machines to do development in different places, and I can't put all unpushed commits in one place and rebase them.)

There are a few ways to deal with that, but I suggest for simplicity that you just always push your work in progress to your remote at the end of your work session. You can then pull it down before you start work on your other system. You can always create a new branch and rebase it before you make the work public. Basically, don't worry if your work-in-progress branches are sloppy, you can always clean them up before you post the changes for review.

Alternatively, I'd use git merge and then rebase the changes, or just rebase one branch onto the other.

And that caused branch diverge and then all kinds of subsequent headaches.

Eventually I used the following approach to consolidate the changes.

E.g. I need to rebase last 4 commits of branch_a and consolidate them into a single commit in branch_b for each better patch submitting:

1. do a rebase for the remote commits (already pushed commits)

   git rebase -i origin/branch_a~4 origin/branch_a 
   

You shouldn't have a detached HEAD after a rebase. To squash 4 commits into 1 you'd just specify "fixup" for the last 3 commits, save and let the rebase run.

2. create a new branch using the detached HEAD (created from step 1)

   git branch branch_b HEAD
   

3. push the new branch to github

   git checkout branch_b
   git push --set-upstream origin branch_b
   

Here is an example of how I'd rebase the t12171.3 branch as you:

$git clone https://github.com/walty8/trac.git trac-walty8-github
Cloning into 'trac-walty8-github'...
remote: Counting objects: 85411, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 85411 (delta 0), reused 0 (delta 0), pack-reused 85409
Receiving objects: 100% (85411/85411), 29.53 MiB | 3.23 MiB/s, done.
Resolving deltas: 100% (67447/67447), done.
Checking connectivity... done.
# Add Edgewall mirror and fetch it
$git remote add mirror https://github.com/edgewall/trac.git
$git fetch mirror
remote: Counting objects: 8246, done.
remote: Compressing objects: 100% (190/190), done.
remote: Total 8246 (delta 3314), reused 3225 (delta 3225), pack-reused 4831
Receiving objects: 100% (8246/8246), 2.62 MiB | 823.00 KiB/s, done.
Resolving deltas: 100% (6511/6511), completed with 989 local objects.
From https://github.com/edgewall/trac
 * [new branch]      0.11-stable -> mirror/0.11-stable
 * [new branch]      0.12-stable -> mirror/0.12-stable
 * [new branch]      1.0-stable -> mirror/1.0-stable
 * [new branch]      1.2-stable -> mirror/1.2-stable
 * [new branch]      trunk      -> mirror/trunk
# Create 1.2-stable and trunk that fetch from the mirror
$git branch 1.2-stable mirror/1.2-stable
Branch 1.2-stable set up to track remote branch 1.2-stable from mirror.
$git branch --set-upstream-to=mirror/trunk trunk
Branch trunk set up to track remote branch trunk from mirror.
# Checkout a new branch
$git checkout -b t12171.4 origin/t12171.3
Branch t12171.4 set up to track remote branch t12171.3 from origin.
Switched to a new branch 't12171.4'
# Rebase on the latest 1.2-stable
$git rebase 1.2-stable
First, rewinding head to replay your work on top of it...
Applying: refs #12171, integrate WikiReportMacro into trac core
...
# Fix conflicts, git add ..., git rebase --continue; repeat as necessary
$git rebase --onto trunk 1.2-stable
First, rewinding head to replay your work on top of it...
Applying: refs #12171, integrate WikiReportMacro into trac core
...
$git push --set-upstream origin t12171.4

Could you give that a try so that we have your changes rebased on the latest trunk? You'll find there are a good number of conflicts. More rebases might be needed before the changes are committed, but this one would at least be good practice.

[Ryan J Ollos]

I've rebased Walty's latest changes and will do the additional refactoring in ReportModule (comment:26). I'm thinking we should name the macro TicketReport, since we already have TicketQuery macro.

[Ryan J Ollos]

The necessary refactoring of ReportModule is more extensive than I expected.

[Ryan J Ollos]

Summarized work in a new ticket, #12810. This ticket got a bit off-topic and it will be easier to revisit with the scope summarized in a new ticket.