Modify

Opened 5 years ago

Closed 5 months ago

Last modified 4 months ago

#6783 closed enhancement (fixed)

RFE: Tags for discussion

Reported by: lhr870630@… Owned by: hasienda
Priority: normal Component: DiscussionPlugin
Severity: normal Keywords:
Cc: rjollos 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 4 years ago.
minimal attempt to tag discussion topics (TagsPlugin integration)
20140424_discussions_tag-provider_rework_1.patch (5.6 KB) - added by hasienda 5 months ago.
1st pass on tracdiscussion/tags.py in 0.11 branch
20140424_discussions_tag-provider_rework_2.patch (5.1 KB) - added by hasienda 5 months ago.
2st pass on tracdiscussion/tags.py in 0.11 branch

Download all attachments as: .zip

Change History (33)

comment:1 Changed 5 years ago by Blackhex

  • Status changed from new to assigned
  • Summary changed from tags for discussion to RFE: Tags for discussion

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

Changed 4 years ago by lucid

minimal attempt to tag discussion topics (TagsPlugin integration)

comment:2 Changed 4 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 4 years ago by Blackhex

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 4 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 3 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 3 years ago by Blackhex

  • Trac Release changed from 0.10 to 0.11

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

comment:7 Changed 3 years ago by JasonWinnebeck

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 5 months ago by hasienda

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 5 months ago by hasienda

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

comment:9 Changed 5 months ago by hasienda

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 5 months ago by hasienda

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

Changed 5 months ago by hasienda

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

comment:11 Changed 5 months ago by hasienda

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 5 months ago by hasienda (previous) (diff)

comment:12 Changed 5 months ago by hasienda

  • Cc rjollos added
  • Owner changed from Blackhex to hasienda

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 5 months ago by hasienda

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 follow-up: Changed 5 months ago by hasienda

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 5 months ago by rjollos

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

comment:16 Changed 5 months ago by hasienda

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 5 months ago by hasienda

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 5 months ago by hasienda

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 5 months ago by hasienda

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 5 months ago by hasienda

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 5 months ago by hasienda

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 5 months ago by hasienda

In 13885:

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

comment:23 Changed 5 months ago by hasienda

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 5 months ago by hasienda

In 13889:

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

comment:25 Changed 5 months ago by hasienda

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

comment:26 Changed 5 months ago by hasienda

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

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 follow-up: Changed 5 months ago by rjollos

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

comment:28 Changed 5 months ago by hasienda

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 5 months ago by hasienda

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 4 months ago by hasienda

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.

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.