RFE: Tags for discussion

[lhr]

I want to use tags in the discussion, could you tell me how can i do it

[Radek Bartoň]

DiscussionPlugin does not have tags support (yet). But 0.10 brach will not have.

[lucid]

minimal attempt to tag discussion topics (TagsPlugin integration)

[lucid]

I would like to second this request. (For Trac 0.12.1 or newer)

If it helps, see the attached discussion-tags.py for my attempt to integrate the DiscussionPlugin with the TagsPlugin. It's only a minimal start to tag discussion topics, and doesn't even work correctly yet. (The TagsPlugin seems to have a problem with the resource realm 'discussion-topic' because of the '-'.)

[Radek Bartoň]

Thank you for the patch. Now I must definetely spent some time to review/apply all your patches and implement pending important feature request :-).

[lucid]

You're welcome. Thank you for working on any of this. Feedback on the patches is welcome. (I'm quite new to trac hacking; There's probably a lot to improve.)

[anonymous]

just chiming in... I'm interested in this as well. I can't actually use the discussion plugin at all because I added a tag to a forum without knowing the plugins were incompatible. Attempting to access a forum gives me this error:

Tags are not supported on the "None" realm

and in the trac debug log: 2011-03-24 15:10:46,600 Trac[api] DEBUG: SELECT id, forum_group, name, subject, time, author, moderators, subscribers, description FROM forum WHERE id = 1 2011-03-24 15:10:46,600 Trac[main] WARNING: HTTPInternalError: 500 Trac Error (Tags are not supported on the "None" realm)

I manually delete'd tag data in the trac.db that referred to the discussion forums but that did not solve them problem.

I'm very new to trac and trac hacking but I'll attempt to play with the attached patch.

[Radek Bartoň]

It's work in progress but currently I don't have much time to finish it.

[Jason Winnebeck]

I think this plugin breaks support for us without the tags plugin installed. Using the latest trunk on both 0.11 and 0.12 Tracs gives a TracError about "TagSystem" not defined or some such. I guessed based on the changelog to revert back to r9787 and I don't see any problems yet.

[Steffen Hoffmann]

While we have (initial?) tagging support in the 0.11 branch, its development is not reflected here.

Furthermore the existence of #8231 suggests, that help is wanted or required with this implementation. Therefore I'll attach some proposed changes and offer to apply them, if co-maintenance is accepted. Critics, more ideas and thoughts welcome.

[Steffen Hoffmann]

1st pass on tracdiscussion/tags.py in 0.11 branch

[Steffen Hoffmann]

summary of changes, 1st pass​:

• enforcing PEP (doc-string formatting, max-char-per-line < 80)
• making imports more explicite (from trac.core)
• merging unnecessarily separated components into one
• correcting permission actions relevant for this plugin
• reducing excessive blank lines and trivial code comments
• making DEBUG log entries more explicite
• simplification of string-replacements (removed iterable notation)

[Steffen Hoffmann]

#8231 for TagsPlugin has been closed as a duplicate of this ticket.

[Steffen Hoffmann]

2st pass on tracdiscussion/tags.py in 0.11 branch

[Steffen Hoffmann]

summary of changes, 2nd pass​:

• clear trivial import comments, remove ununsed imports
• use TagSystem object as class attribute instead of repeated instantiation
• add missing describe_tagged_resource() method
• remove tag list sort in private methods by returning tags in sets - the default interable object returned by TagsPlugin's own tag providers
• reduce method count by moving code of _get_stored_tags() and _delete_tags() into change listener methods
• add ToDo markers for unhandled issues found during code review

[Steffen Hoffmann]

Ryan, would you have a look at it, please.

I'd like to get agreement on these changes, before I take more actions. I'll try hard to get this tag provider and surrounding code into better shape. Just make sure to write a short comment, even if you see no issues. Thanks in advance.

[Steffen Hoffmann]

In 13880:

DiscussionPlugin: Initial set of changes to improve existing tag provider, refs #6783.

Changes include

• enforcing PEP (doc-string formatting, max-char-per-line < 80)
• making imports more explicite (from trac.core)
• merging unnecessarily separated components into one
• correcting permission actions relevant for this plugin
• reducing excessive blank lines and trivial code comments
• making DEBUG log entries more explicite
• simplification of string-replacements (removed iterable notation)

[Steffen Hoffmann]

To recap an email correspondence aside of this ticket comment flow, there has been a developer discussion involving rjollos into the decision on proceeding with some non-maintainer changes by me. Due to obvious needs for improvements we agreed to proceed according to my proposals.

Initial testing before [13880] revealed an issue, that could be reproduced like follows:

1. create a new forum
2. create a new forum group
3. goto the forum admin panel

Selecting the new forum (associated with forum group 'None') for edit raises

WARNING: [<ip-addr-removed>] HTTPInternalError: 500 Trac Error
 (Tags are not supported on the 'None' realm)

This is already fixed by the first clean-up. Obviously I did a little more than just cleaning it up, nice. More coming soon.

[Ryan J Ollos]

Thanks, Steffen. The changes look good to me. I'll try to find some time to provide additional feedback via testing.

[Steffen Hoffmann]

It turned out, that using TagSystem object as class attribute instead of repeated instantiation was not such a bright idea: It yielded an infinite loop on init, because of the tag provider triggering tag system instantiation and vice versa. So this part is dropped from the set of coming changes.

[Steffen Hoffmann]

In 13881:

DiscussionPlugin: Review and refit of tag provider, refs #6783.

Change summary:

• clear trivial import comments, remove ununsed imports
• add missing describe_tagged_resource() method
• remove tag list sort in private methods by returning tags in sets - the default interable object returned by TagsPlugin's own tag providers
• reduce method count by moving code of _get_stored_tags() and _delete_tags() into change listener methods
• add ToDo markers for unhandled issues found during code review
• fix check_permission() broken by reflowing super() call in [13880]

[Steffen Hoffmann]

In 13882:

DiscussionPlugin: Major unification of timeline discussion event provider code, refs #6783.

Replacing string tuples with true Trac Resource objects is required i. e.
to let this event provider work with tractags.web_ui.TagTimelineEventFilter.

Other changes include

• more imports cleanup and PEP8 changes (line-wrap, indentation)
• making DEBUG log entries more explicit
• simplification of string-replacements and refit of component doc-string according to component relevance markup proposal (see #11690 for TagsPlugin)
• more readable 'message_<messageID>' anchor name format for direct links to individual messages (was: 'message<messageID>')

[Steffen Hoffmann]

It looks like topics and attachments as well as messages are in a parent-child-relation here. If we follow this concept, we may re-define

• topic-group as a parent resource of topics
• name actions to let attachments and messages follow topics 'reparenting'

later on too.?

Review and thoughts on the concept welcome.

[Steffen Hoffmann]

Note to myself: I remember that I removed an occurrence of 'if..else', a Python2.5 coding paradigm and as such a typical Trac 0.11 backwards-compatibility breaker, from get_timeline_events().

I try to follow my current preference bringing back and keeping Trac 0.11 compatibility before forking into a 1.0 branch for adoption of new Trac db API. While going through more of this plugin's code I'll have to watch out for more such issues.

[Steffen Hoffmann]

Replying to hasienda:

WARNING: [<ip-addr-removed>] HTTPInternalError: 500 Trac Error
 (Tags are not supported on the 'None' realm)

This reappeared somehow, will have to address it explicitly, likely as part of a closer review of tracdiscussion/admin.py.

[Steffen Hoffmann]

In 13885:

DiscussionPlugin: Cleanup for the admin panel provider, refs #6783.

[Steffen Hoffmann]

In 13887:

DiscussionPlugin: Clean-up for environment setup participant, refs #6783.

Most notably passing a db cursor outside of the scope of the constructing
method or function shall be avoided, because it causes rather nasty,
hard-to-debug issues. Rephrasing 'db' to 'schema' for clarity.

We do not handle exceptions on db upgrade but let Trac deal with it instead,
enabling it to better care itself for side-effects on other setup participants.
Introducing some INFO logging gives a bit more insight behind the scene of
running upgrade actions.

[Steffen Hoffmann]

In 13889:

DiscussionPlugin: Clean-up for the core request handler, refs #6783.

[Steffen Hoffmann]

[13889] was a bit more than just PEP8, even adding missing import for add_link().

[Steffen Hoffmann]

DiscussionPlugin works well with tractags>=0.7, so the issue is clearly resolved.

See #11706 for more non-maintainer activity bringing the plugin into better shape.

[Ryan J Ollos]

Should we bump the minimum required version of TracTags to 0.7?: discussionplugin/0.11/setup.py@13886:43#L15​

[Steffen Hoffmann]

In 13899:

DiscussionPlugin: Require tractags-0.7, because we build on 0.7 API and older versions are depreciated anyway, refs #6783.

[Steffen Hoffmann]

Replying to rjollos:

Should we bump the minimum required version of TracTags to 0.7?

Totally agreed, thank you for the reminder.

[Steffen Hoffmann]

In 13939:

DiscussionPlugin: Change listeners got incomplete objects, refs #6783 and #8981.

Fixed tag-related change listeners to get discussion item IDs, what caused
KeyErrors before, just raised a couple of other issues. Not all of these
issues are resolved yet. I decided to initially catch listener errors with
logged warnings for improved API method robustness.

Moved time stamp generation nearer to db action and letting _add_item()
return the added item ID obsoleted four api methods at once.