Modify

Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#6783 closed enhancement (fixed)

RFE: Tags for discussion

Reported by: lhr Owned by: Steffen Hoffmann
Priority: normal Component: DiscussionPlugin
Severity: normal Keywords:
Cc: Ryan J Ollos Trac Release: 0.11

Description

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

Attachments (3)

discussion-tags.py (3.2 KB) - added by lucid 7 years ago.
minimal attempt to tag discussion topics (TagsPlugin integration)
20140424_discussions_tag-provider_rework_1.patch (5.6 KB) - added by Steffen Hoffmann 3 years ago.
1st pass on tracdiscussion/tags.py in 0.11 branch
20140424_discussions_tag-provider_rework_2.patch (5.1 KB) - added by Steffen Hoffmann 3 years ago.
2st pass on tracdiscussion/tags.py in 0.11 branch

Download all attachments as: .zip

Change History (33)

comment:1 Changed 8 years ago by Radek Bartoň

Status: newassigned
Summary: tags for discussionRFE: Tags for discussion

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

Changed 7 years ago by lucid

Attachment: discussion-tags.py added

minimal attempt to tag discussion topics (TagsPlugin integration)

comment:2 Changed 7 years ago by 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 '-'.)

comment:3 Changed 7 years ago by 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 :-).

comment:4 Changed 7 years ago by 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.)

comment:5 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by Radek Bartoň

Trac Release: 0.100.11

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

comment:7 Changed 6 years ago by 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.

comment:8 Changed 3 years ago by 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.

Changed 3 years ago by Steffen Hoffmann

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

comment:9 Changed 3 years ago by 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)

comment:10 Changed 3 years ago by Steffen Hoffmann

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

Changed 3 years ago by Steffen Hoffmann

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

comment:11 Changed 3 years ago by 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
Last edited 3 years ago by Steffen Hoffmann (previous) (diff)

comment:12 Changed 3 years ago by Steffen Hoffmann

Cc: Ryan J Ollos added; anonymous removed
Owner: changed from Radek Bartoň to 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.

comment:13 Changed 3 years ago by 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)

comment:14 Changed 3 years ago by 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.

comment:15 Changed 3 years ago by Ryan J Ollos

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

comment:16 Changed 3 years ago by 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.

comment:17 Changed 3 years ago by 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]

comment:18 Changed 3 years ago by 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>')

comment:19 Changed 3 years ago by 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.

comment:20 Changed 3 years ago by 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.

comment:21 in reply to:  14 Changed 3 years ago by 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.

comment:22 Changed 3 years ago by Steffen Hoffmann

In 13885:

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

comment:23 Changed 3 years ago by 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.

comment:24 Changed 3 years ago by Steffen Hoffmann

In 13889:

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

comment:25 Changed 3 years ago by Steffen Hoffmann

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

comment:26 Changed 3 years ago by Steffen Hoffmann

Resolution: fixed
Status: assignedclosed

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.

comment:27 Changed 3 years ago by Ryan J Ollos

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

comment:28 Changed 3 years ago by Steffen Hoffmann

In 13899:

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

comment:29 in reply to:  27 Changed 3 years ago by Steffen Hoffmann

Replying to rjollos:

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

Totally agreed, thank you for the reminder.

comment:30 Changed 3 years ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.