Modify

Opened 3 years ago

Closed 5 months ago

#9059 closed enhancement (fixed)

[PATCH] Various fixes and enhancements

Reported by: anonymous Owned by: hasienda
Priority: normal Component: TagsPlugin
Severity: normal Keywords: admin configuration option query presentation i18n
Cc: itamarost@…, rjollos, otaku42 Trac Release: 0.12

Description

One ticket for the patch.

Additional tickets to be created for each specific fix / enhancement.

Attachments (1)

tagsplugin-misc-enhancements.patch (21.8 KB) - added by itamarost 3 years ago.
patch against trunk at r10506

Download all attachments as: .zip

Change History (34)

comment:1 Changed 3 years ago by anonymous

ough...
I'm unable to upload the patch (rejected by spam filter)...

will get back to it once I find some place to upload and share.

Changed 3 years ago by itamarost

patch against trunk at r10506

comment:2 Changed 3 years ago by itamarost

  • Cc itamarost@… added
  • Trac Release changed from 0.11 to 0.12

Successfully attached!

Referred tickets: #9060, #9061, #9062, #9063, #9064

Includes existing patches from: #8638,#3754

comment:3 Changed 3 years ago by hasienda

  • Cc rjollos otaku42 added
  • Status changed from new to assigned

Wow, thanks. That's a bunch of improvements, really a master ticket.

I'll review this and certainly apply as a series of changes again. Nevertheless it's good to have code for all the single patches rebased on current trunk.

Thanks for breaking it into discrete issues. And I do especially appreciate your non-anonymous contributions. :-)

comment:4 Changed 3 years ago by osimons

Nice - the plugin deserves som TLC :-) Looks good to me, and the wiki rename methods will of course also be compatible with 0.11 as the method will just exist but never be called.

One minor nit: Interface class declarations in Trac (and plugins) are always defined without adding self to the template methods. It has no real functional meaning, other than established practice to specify the essence of the API without implementation details + prevent that someone by accident makes an instance of the Interface class and tries to call it (if so it should fail).

comment:5 Changed 3 years ago by anonymous

Sorry for the anonymity... My only internet-access at the moment is the Black Hat WiFi (security conference, so be sure the network is heavily monitored), and the t-h site is configured with basic authentication (password in cleartext), so I rather *not* login... :-)

Also sorry about all the self args. It bothered my coding style...

comment:6 Changed 3 years ago by hasienda

(In [10621]) TagsPlugin: Control default link destination in tags cloud, refs #3754 and #9059.

Known behavior to always link to an existing wiki page with name equal to the
tag is still default, but configurable by new wiki_page_link boolean option.

comment:7 Changed 3 years ago by hasienda

(In [10622]) TagsPlugin: Cleanup of imports and other stuff, refs #9059.

Hide TagTemplateProvider providing the ITemplateProvider for being an
abstract class, and remove mentioning of ClearSilver templates.
Old author email added to display something at Trac 0.11 plugin admin page.

comment:8 Changed 3 years ago by hasienda

(In [10624]) TagsPlugin: Adapt REGEXP for 1st title extraction to new Trac 0.12 wiki syntax, refs #9059 and #9062.

Report and fix have been contributed by Itamar Ostricher, thank you.

comment:9 Changed 3 years ago by hasienda

(In [10627]) TagsPlugin: Improve permission definition for more intuitive permission settings, refs #9058 and #9059.

Add a third permission TAGS_ADMIN for access to future 'tags' admin panel.

comment:10 Changed 3 years ago by hasienda

(In [10629]) TagsPlugin: Add deduplication capability to tags query, refs #9059 and #9060.

This has been tested to fix both, the tags query command line exposed in
'tags' main page as well as ListTagged() macro calls with 2 or more
repetitions of the same resource realm.

Thanks - again to Itamar Ostricher - for test/report and proposed fix.

comment:11 Changed 3 years ago by hasienda

(In [10632]) TagsPlugin: Implement IWikiChangeListener interface for Trac 0.12, refs #8638 and #9059.

This work is based on code contributed/tested by Itamar Ostricher and sika.

Had a hard time myself to understand the consequences of wiki rename in
Trac 0.12 i.e. for attachments, but now I favor this approach, even regarding
naming conventions.

So I preferred reparent_resource_tags over rename_resource because the
former is what we actually do while the latter has already been done, when
IWikiChangeListener method wiki_page_renamed is triggered. Other details
regarding the latest patch by Itamar have been discussed in #9059 before.

comment:12 Changed 3 years ago by hasienda

(In [10636]) TagsPlugin: A spurious self argument slipped in into changeset [10632], refs #8638 and #9059.

As outlined by Odd (osimons) in comment 4 of #9059
the usual first argument self is omitted from Interface definitions
intentionally; cite:

Interface class declarations in Trac (and plugins) are always defined
without adding self to the template methods. It has no real functional
meaning, other than established practice to specify the essence of the API
without implementation details + prevent that someone by accident makes an
instance of the Interface class and tries to call it (if so it should fail).

comment:13 Changed 3 years ago by hasienda

(In [10637]) TagsPlugin: Refactor author retrieval from tag change request, refs #9059 and #9061.

Code threw an AttributeError: username exception when attempting to change
tags on tickets. Couldn't find another occurance of req.username in any
other plugin nor in Trac core itself.

Turns out this flaw has been specific to the 0.6 release introduced by the
first development code in changeset [2953] back in 2008 - wow. I stumbled
upon this when attempting to implement tag mass replacement - a lucky catch.
Lucky again for finding get_reporter_id, that has been avaiable since
earliest days - 2004, and it works very well.

comment:14 Changed 3 years ago by hasienda

(In [10639]) TagsPlugin: Add tag mass replacement functionality, refs #9058, #9059 and #9061.

This is a modified version of code originally contributed by Itamar Ostricher.
I felt like improving usability by replacing the input field for old tag(s)
with a select box containing all currently defined tags, where even multiple
selection and tag mass deletion is possible now.

Thank you for your truely inspiring work, Itamar.

comment:15 Changed 3 years ago by hasienda

(In [10661]) TagsPlugin: Provide configurable default list of realms to exclude from tags query, refs #9059 and #9063.

Administrators may configure tag realms to require explicite selection for
every new tags query by adding them to a new ListOption 'exclude_realms'
or 'listtagged_exclude_realms' for ListTagged macro calls respectively.
The original suggestion has been contributed by Itamar Ostricher, thank you.

These changes include an additional workaround to get the expected result,
even if you deselect all available realms. Otherwise this would have
neutralized your setting for these options.

comment:16 Changed 3 years ago by hasienda

The remaining changes as referred to in #9064 are non-trivial again. I'm still struggling to get them right. And there are some more issues that got my attention along the way; will keep you updated as I progress.

Hope the results of my "patch-picking" do satisfy the extra waiting time. Thanks for your patience anyway.

comment:17 follow-ups: Changed 3 years ago by hasienda

  • Keywords admin configuration option query presentation i18n added

As denoted in #9064 the final commit for this patch set is getting near.

Any more thoughts for next steps towards tags-0.7? I'll try to keep at least the current pace and go through some more tickets assigned to this plugin that I feel the need to resolve before declaring next stable release. Advice/guidance on this is always welcome. Just comment existing tickets appropriately, please.

Probably one of the most pressing, but undocumented issues is backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by osimons

Replying to hasienda:

Probably one of the most pressing, but undocumented issues is backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

That is not ideal. Ideally the same code should work for both 0.11 and 0.12 as there is no branch alternative either. Making various compat & noop tweaks should not be so hard - it is what I try to do for all my plugins.

A start would be to wire up unit tests for the plugin. It really is somewhat overdue... I can just commit this if you'd like.

comment:19 in reply to: ↑ 18 Changed 3 years ago by hasienda

Replying to osimons:

Replying to hasienda:

![...] backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

That is not ideal. Ideally the same code should work for both 0.11 and 0.12 as there is no branch alternative either. Making various compat & noop tweaks should not be so hard - it is what I try to do for all my plugins.

I know. But at the time of first implementation I didn't know better and there was no 'best practice' to follow. And I had no own 0.11 test install to check on my own. All this changed and I've already found ways around the various issues for AccountManagerPlugin before. So this is just checking for possible similar glitches and fixing them by applying similar code changes (at least in theory). I'm not afraid of this. Can do that, as my time permits.

A start would be to wire up unit tests for the plugin. It really is somewhat overdue... I can just commit this if you'd like.

Took me some time to understand the strong points of a solid unittest framework with good coverage. Yes, feel free to go ahead with #9194, please. I'll certainly watch your steps to learn more for myself. There are many more plugins that lack any systematic code testing.

comment:20 Changed 3 years ago by hasienda

(In [10680]) TagsPlugin: Provide default sets of two new options for both, ListTagged and /tags handler, refs #9059 and #9064.

The TagQuery result presentation is now configurable. Supported values for
the format keyword argument are:

  • oldlist - list in known ListTagged style
  • compact - list with results shortend to just the description
  • table - table view similar to TicketQuery with 'table' option

and for 'table' view columns can be selected with the cols argument from a

frozenset(['realm', 'id', 'description', 'tags'])

separating multiple values by pipe character ('|').

While these are great new features I wanted myself before, it would not have
happend by now without the contribution from Itamar Ostricher. Thanks a bunch.

comment:21 Changed 3 years ago by hasienda

(In [10681]) TagsPlugin: Fix system-message-like banner insert, refs #9059.

First, cleanup superfluous part of a REGEXP. But it turned out, that adding
the message while already rendering doesn't work, not even for wiki pages.
So I've changed the whole concept for message insertion.

comment:22 Changed 3 years ago by hasienda

(In [10682]) TagsPlugin: Rework TicketTagProvider, refs #9059.

With i18n there's no point in hard-coding labels for tagged resource display.
It's not sensible to reinvent the wheel too, so describe_tagged_resource
method for tickets just calls the corresponding module in Trac core,
and we'll get good localization/translations of labels for free.

This is the final part of bundled suggestions contributed by Itamar Ostricher.
Thank you for all the invaluable ground work and inspiration.

comment:23 Changed 3 years ago by hasienda

(In [10683]) TagsPlugin: Start tracking changes more visibly, i.e. to serve summary on releases, refs #9059.

Adding changes since tractags-0.6 to prepare for next release, but regarding
earlier plugin versions this is merely a stub. Please correct, if you know
better.

comment:24 in reply to: ↑ 17 Changed 3 years ago by rjollos

Replying to hasienda:

Probably one of the most pressing, but undocumented issues is backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

I'm running Trac 0.11.7 and r10671 of the TagsPlugin is working well. However, following [10680] I can no longer run the latest version of the TagsPlugin trunk; I get errors about the inability to import ChoiceOption. That class doesn't look familiar to me so I'm assuming it must have been added in 0.12 or later.

2011-09-25 19:06:50,685 Trac[loader] ERROR: Skipping "tractags = tractags":
(can't import "ImportError: cannot import name ChoiceOption")

comment:25 Changed 3 years ago by osimons

It causes the test suite to crash for 0.11.x runs too. Perhaps you should make a virtualenv with Trac 0.11.x? No need to configure web server or things like that, just install Babel, Genshi and Trac and you'll have a python to use for running tests...

comment:26 follow-up: Changed 3 years ago by hasienda

Thanks for the feedback. Sure, I've not checked this before, because I've been quite sure there would be some compatibility issues.

Step-by-step, first the original patches reworked, now the backwards compatibility. I've already created some virtualenv's for Trac 0.11 and 0.11.8 to check other plugins, so will resolve this soonish. Just felt like everything at once would have been too much for me, sorry.

This specific option has been added late in Trac 0.12 development, near release time. I could create an import from future. Fairly easy to implement as additional code handling the ImportError. But worth the hassle? I think NO, but KISS instead:

The plugin will not be 'degraded' much, if we resort to ListOption instead. ChoiceOption or not, as discussed in #9064 before, code is currently designed to behave the same as with value 'oldlist' in case of an invalid value, so I can't see how one would mess up anything with this option anyway. We just loose the added functionality of reporting back valid options.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 3 years ago by rjollos

Replying to hasienda:

Thanks for the feedback. Sure, I've not checked this before, because I've been quite sure there would be some compatibility issues.

No problem on my end ... happy to test whatever you push to the trunk. Worst case, I just roll back to a previous version. I wish I could contribute more in terms of attempted patches, but I seem to have no free time lately ... at least until my company gets its product out and I finish up this other contract job.

Is your plan to support the last two versions of Trac? ... i.e. when Trac 0.13 is released, drop support for 0.11?

comment:28 in reply to: ↑ 27 Changed 3 years ago by hasienda

Replying to rjollos:

No problem on my end ... happy to test whatever you push to the trunk. Worst case, I just roll back to a previous version. ![...]

Ok, I'll push something out soon. Feels good to have some faithful (close) followers. :-)

Is your plan to support the last two versions of Trac? ... i.e. when Trac 0.13 is released, drop support for 0.11?

Not yet. As stated at other occasions, the effort to keep compatibility including Trac versions down to 0.11 is reasonable regarding seemingly widespread use. Not forcing Trac upgrade, just to get latest improvements of plugins should be general policy - at least I think that way.

And Trac 0.13 doesn't break 0.11 stuff by now, AFAIK, so I think the next branch will be beyond 0.13, but than for sure at least to no longer ignore @with_transaction & Co. and get rid of the old db handling, more capable context/formatting code, maybe more...

comment:29 Changed 3 years ago by hasienda

(In [10707]) TagsPlugin: Withdraw ChoiceOption for Trac 0.11 backwards-compatibility, refs #9059.

comment:30 Changed 3 years ago by hasienda

(In [10708]) TagsPlugin: Provide fallback for non-existing TicketSystem attribute, refs #9059.

The previous change was just enought to uncover another issue, this time
related to a new Trac 0.12 attribute. But seems not hard to fix that as well.

comment:31 Changed 3 years ago by rjollos

I'm running r10708 on Trac 0.11.7 and it seems to be all working correctly.

comment:32 Changed 15 months ago by hasienda

(In [13235]) TagsPlugin: Prevent TypeError for ListTagged macros without arguments, refs #9059.

Only [[ListTagged()]] worked before, now [[ListTagged]] does as well.
This regression was introduced in [10681].

comment:33 Changed 5 months ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

In 13815:

TagsPlugin: Completing preparation for v0.7 release.

Availability of that code as stable, tagged release
closes #2429, #3359, #3610, #3624, #3677, #3754, #3864, #3947, #3983, #4078, #4277, #4503, #4799, #5523, #7787, #7857, #8638, #9057, #9058, #9059, #9060, #9061, #9062, #9063, #9149, #9210, #9521, #9630, #9636, #10032, #10416, #10636, #11096, #11147, #11152, #11274, #11302, #11658 and #11659.

Additionally there are some issues and enhancement requests showing progress,
but known to require more work to resolve them satisfactorily, specifically
refs #2804, #4200, #8747 and #9064.

Thanks to all contributors and followers, that enabled and encouraged a good
portion of this development work.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.