Modify

Opened 5 years ago

Last modified 3 years ago

#5907 assigned enhancement

[Patch] The `helppage` field should transform the Keywords field into a link to an arbitrarily defined wiki page

Reported by: rjollos Owned by: hasienda
Priority: high Component: KeywordSuggestPlugin
Severity: normal Keywords: link option
Cc: otaku42 Trac Release: 0.11

Description

It would be a nice enhancement if the helpage parameters had behavior such that setting it to point to a wiki page would transform the Keywords field on the ticket form into a link to that page.

For example, I'd like to set helppage = /tags and have the Keywords field transformed to a link that point to the /tags page.

Attachments (4)

keywordsuggestplugin-helppage.patch (1.4 KB) - added by abompard 4 years ago.
keywords-link-resource.patch (3.2 KB) - added by hasienda 3 years ago.
some improvements over current, backwards-incompatible state of handling helppage option
keywords-link-resource.2.patch (3.7 KB) - added by hasienda 3 years ago.
another preliminary development state with some improvements
keywords-link-resource.3.patch (5.6 KB) - added by hasienda 3 years ago.
more improvements: private method _get_helppage_link and backwards-incompatibility down to Trac 0.11

Download all attachments as: .zip

Change History (29)

comment:1 follow-ups: Changed 5 years ago by yoheeb@…

I have actually made this enhancement. It doesn't specifically handle the case if the keywords = field is left blank, as I actually use it. however I will look at that modification also. It is using trac 11.5 with the patch in #4201 attached on 12/04/08

It is literally 5 characters different: on line 73 of keywordsuggest.py:

-                link = tag.a(href='%s' % helppage, target='blank')
+                link = tag.a(href='wiki/%s' % helppage, target='blank')

Then, in my trac.ini file, my helppage link is:

[keywordsuggest]
helppage = tags
keywords = tag1,tag2

this is line 73 of the merged patch to use the tags in keywordsuggest patch in #4201 (I used the whole file replacement 5.7kb option)

I don't think it's enough of a change to make a patch file at this point, but if I address the other issue, I will.

Question is, does this change belong with the tags patch in #4201, or with the main plugin code? I did it this way, so I could, instead if so desired, use a global keyword help page, since I am set up in a multi-environment trac configuration, and they all use the same basic keyword model. The downside, I suppose is that if you do want a wiki page, you have to use wiki/keywords or similiar, not a terrible burden

comment:2 in reply to: ↑ 1 Changed 5 years ago by anonymous

Replying to yoheeb@gmail.com:

Then, in my trac.ini file, my helppage link is:

[keywordsuggest]
helppage = tags
keywords = tag1,tag2

CORRECTION: sorry, I made a typo:
the helpage entry is:

  • /ParentEnv/ProjectPage/tags

since I am multi-env and don't have all my paths set up. The above worked in a scenario where I had some funky apache stuff going on. Sorry about that, it should be:

[keywordsuggest]
helppage = /ParentEnv/ProjectPage/tags
keywords = tag1,tag2

comment:3 in reply to: ↑ 1 Changed 5 years ago by yoheeb@…

Replying to yoheeb@gmail.com: I am not sure why the diff in the original looks like that (I just pasted) but I think it should be reversed (just get rid of wiki/). Either that or I need more coffee!:

-                link = tag.a(href='wiki/%s' % helppage, target='blank')
+                link = tag.a(href='%s' % helppage, target='blank')

comment:4 Changed 4 years ago by rjollos

I'll go ahead and make this change pending feedback on #8156.

comment:5 Changed 4 years ago by rjollos

  • Summary changed from The `helppage` field should transform the Keywords field into a link to an arbitrarily defined wiki page to [Patch] The `helppage` field should transform the Keywords field into a link to an arbitrarily defined wiki page

comment:6 Changed 4 years ago by rjollos

  • Priority changed from normal to high

comment:7 Changed 4 years ago by rjollos

  • Owner changed from scratcher to rjollos
  • Status changed from new to assigned

comment:8 follow-up: Changed 4 years ago by abompard

I have a (IMHO) better version of the patch, which works even when Trac is not at the root of the virtualhost. It uses Trac's own resource-to-link converter. I'll attach it to this ticket.

Changed 4 years ago by abompard

comment:9 in reply to: ↑ 8 Changed 3 years ago by rjollos

  • Cc hasienda added; anonymous removed

Replying to abompard:

I have a (IMHO) better version of the patch, which works even when Trac is not at the root of the virtualhost. It uses Trac's own resource-to-link converter. I'll attach it to this ticket.

It looks like your patch wouldn't allow, e.g. tags to be specified for the help page, since it is not in the wiki realm. If you take a look at the patch I'm about the commit and have any suggestions for improvement, I'll follow-up.

comment:10 follow-up: Changed 3 years ago by rjollos

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

(In [10936]) Fixes #5907: Version 0.4.1: A regression in the 0.4 version of the plugin prevented the helppage link from displaying when helppage.newwindow was False.

The helppage link can now be specified as a resource in any realm, not just a wiki page.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by hasienda

  • Keywords link option added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to rjollos:

The helppage link can now be specified as a resource in any realm, not just a wiki page.

True. This has actually been started in [10431] by removing code to always add the mandatory 'wiki/' prefix. I suggest to treat this as a regression bug on it's own, because tickets claiming this are foreseeable, right?

Still I like more flexibility and existing code rules over just wild ideas. So consider the following patch for improving the situation.

Changed 3 years ago by hasienda

some improvements over current, backwards-incompatible state of handling helppage option

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

Replying to hasienda:

True. This has actually been started in [10431] by removing code to always add the mandatory 'wiki/' prefix. I suggest to treat this as a regression bug on it's own, because tickets claiming this are foreseeable, right?

Still I like more flexibility and existing code rules over just wild ideas. So consider the following patch for improving the situation.

Agreed. Thank you for your attention to backward compatibility and for adding additional functionality.

I'd like to extract the code from your patch into a private method __get_helppage_link to allow for unit testing in the future, and to keep the main filter_steam method concise and readable. This is the programming approach I'd take in general, but given your much stronger knowledge of Python and Trac, I wanted to check with you before making this change.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by hasienda

Replying to rjollos:

Agreed. Thank you for your attention to backward compatibility and for adding additional functionality.

You're welcome. Glad you like it.

I'd like to extract the code from your patch into a private method __get_helppage_link ... but ... I wanted to check with you before making this change.

Sure, valid move, even more after my proposed extensions. But let me go ahead and attach the current patch as another preliminary development state:

  • changed order of processing to exclude any (explicitly or assumed) wiki resource from being checked - rationale: I'd like to be able to create the configured wiki page, if not existent.
  • idea salvaged from abompard's patch: possibility to use an anchor (wiki-only) - rationale: Special care must be taken to prevent it from getting urlencoded inside the Href object.
  • interpreting whole string as wiki page ID is an intentional fallback, if resource validation fails, because
    1. the resource realm is unknown or
    2. the resource with the given ID doesn't exist in that realm

Please discuss, if anything is questionable or undesired from your point of view.

I'll create the private method now and attach a third patch for review.

Changed 3 years ago by hasienda

another preliminary development state with some improvements

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

Replying to hasienda:

Please discuss, if anything is questionable or undesired from your point of view.

Only thing that jumps out initially is that the first 2 if statements might be transformed to an if-elif, dropping the if not link check on the second.

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

Replying to rjollos:

Only thing that jumps out initially is that the first 2 if statements might be transformed to an if-elif, dropping the if not link check on the second.

That is, unless req.href can return None, but I'm not familiar with a condition in which this can occur.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by hasienda

Replying to rjollos:

Replying to rjollos:

Only thing that jumps out initially is that the first 2 if statements might be transformed to an if-elif, dropping the if not link check on the second.

That is, unless req.href can return None, but I'm not familiar with a condition in which this can occur.

Me too, but I can't map your comment to current code now. Maybe it's just too late, anyway, let me drop in the rework towards a private method.

Patch has been tested on Trac 0.11 and 0.13dev-r10880, so I noticed another issue: recently added javascript_quote from trac.util.text is not available before Trac 0.11.3 . Still like full backwards-compatibility? Suggestion included in the following patch.

Changed 3 years ago by hasienda

more improvements: private method _get_helppage_link and backwards-incompatibility down to Trac 0.11

comment:17 Changed 3 years ago by hasienda

Forgot: Please make sure to include explicit file encoding ('utf-8') as suggested in latest patch, thanks.

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

Replying to hasienda:

Patch has been tested on Trac 0.11 and 0.13dev-r10880, so I noticed another issue: recently added javascript_quote from trac.util.text is not available before Trac 0.11.3 . Still like full backwards-compatibility? Suggestion included in the following patch.

Certainly. I'll take the code since your have generously provided it, though I probably wouldn't have gone out of my way to do it myself up until now.

I've been developing on 0.11.7 and testing on 0.12.2, which I had felt gave me sufficient coverage to satisfy most users. I have a dream of setting up automated builds to test installation and perhaps run unit tests against a greater range of versions, but I'm a ways off from that. Until then, I might modify my approach and start developing against 0.11.0, like you are doing, at least for plugins that have been around quite a while such as this one.

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

A note of caution: 0.13dev has great auto-preview like wiki side-by-side editor in tickets, now not only for comments but all fields.

But this preview interferes with plugin JS code, because it uses redirection internally. So I can't see the drop-down list anymore. Seen functional regression in other plugins as well, i.e. TracTicketTemplatePlugin.

Will investigate and most probably open another ticket dedicated to 0.13 compatibility - no need to do that in 0.11 branch I guess.

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

  • Cc otaku42 added; hasienda removed

Replying to rjollos:

... I have a dream of setting up automated builds to test installation and perhaps run unit tests against a greater range of versions, but I'm a ways off from that.

Me too, once again.

<wild-dream> Super-cool, if this could even become a centralized service a-la ci@trac-hacks.org, to provide independent insight into code quality with a few mouse clicks.</wild-dream>

comment:21 in reply to: ↑ 19 Changed 3 years ago by rjollos

Replying to hasienda:

Will investigate and most probably open another ticket dedicated to 0.13 compatibility - no need to do that in 0.11 branch I guess.

Yeah, that is definitely appreciated. I'm actually surprised I didn't destroy 0.11 compatibility with the jQuery UI changes made in #8141. After I made those changes, I was thinking this would be the case, and I'd have to rewind some changes on the 0.11 branch and start a trunk version or at least a 0.12 branch. I did break compatibility with the AutocompleteUsersPlugin. I intend to fix this issue in #9599.

comment:22 Changed 3 years ago by rjollos

This never seems to get high enough on my priority list lately for me to test and commit the patch. Steffen, if convenient for you, feel free to push your patch straight to the repository. You have repository-wide commit access now so this is possible, and I trust your work!

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

  • Owner changed from rjollos to hasienda
  • Status changed from reopened to new

Ok, will take care, but IIRC you did some change in-between, so I'll have to adapt/test/re-validate.

Splitting the latest path in two should do nicely: Add compat-function, and in another step add the flexible option parser code. Thanks for the thumbs-up.

comment:24 Changed 3 years ago by hasienda

(In [11201]) KeywordSuggestPlugin: Check helppage config option for valid realm ID, refs #5907.

Try Trac's resource system to create appropriate links to any existing resource. We gain backwards-compatibility by continuing to assume a wiki resource, if options value has no leading /.

comment:25 in reply to: ↑ 23 Changed 3 years ago by hasienda

  • Status changed from new to assigned

Replying to hasienda:

Ok, will take care, but IIRC you did some change in-between, so I'll have to adapt/test/re-validate.

Splitting the latest path in two should do nicely: Add compat-function, and in another step add the flexible option parser code. Thanks for the thumbs-up.

Done, so we have all that I was speaking about in another comment before. Now it is a very versatile, backwards-compatible option and allows for virtually any possible customization to the 'Keywords' labeled link. Feedback welcome.

Add Comment

Modify Ticket

Action
as assigned The owner will remain hasienda.
Author


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

 
Note: See TracTickets for help on using tickets.