Modify

Opened 10 years ago

Closed 7 years ago

#11706 closed task (fixed)

Code review

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

Description

As recent development has been tracked in #6783, but going totally out-of-scope now, this ticket is meant to collect references and additional comment regarding this ongoing effort for tracdiscussion-0.9.

Attachments (0)

Change History (38)

comment:1 Changed 10 years ago by Steffen Hoffmann

In 13890:

DiscussionPlugin: Script header/imports cleanup, refs #11706.

More PEP8 changes (doc-string quotation placement, line-wrap), comment review,
and complete preparation of URLs to messages as anchors in the corresponding
topic (parent resource) page by get_resource_url() for the discussion
Trac Resource domain as planned in [13882].

comment:2 Changed 10 years ago by Steffen Hoffmann

In 13891:

DiscussionPlugin: Add license headers to python scripts making license more obvious as per Trac plugin coding recommendations, refs #11706.

comment:3 Changed 10 years ago by Steffen Hoffmann

In 13893:

DiscussionPlugin: Add license header to Genshi HTML templates making license more obvious here too, refs #11706.

comment:4 Changed 10 years ago by Steffen Hoffmann

In 13909:

DiscussionPlugin: Create automated test harness for this plugin, refs #11706.

Arrange unittest infrastructure for all modules, including test methods
with a couple of component-initialization-only tests for now.

Tests can be run either as the full suite:

$> PYTHONPATH=. python setup.py test

or by just running a single test module:

$> PYTHONPATH=. python -m tracdiscussion.tests.api

comment:5 Changed 10 years ago by Steffen Hoffmann

In 13910:

DiscussionPlugin: Cleanup spam filter adapter component, refs #11706.

comment:6 Changed 10 years ago by Steffen Hoffmann

In 13915:

DiscussionPlugin: Move db access methods into separate module, refs #11706.

This is the initial split as done to other plugins before for improved
maintainability of db schema-specific code. Cutting method count per class
as well as overall lines per class down to more manageable size will help
anyway.

comment:7 Changed 10 years ago by Steffen Hoffmann

In 13916:

DiscussionPlugin: Correct issues with tag retrieval, refs #756 and #11706.

Resource construction must happen before its use in a tag provider method.
An additional private method enables code deduplication for forum data
retrieval.

comment:8 Changed 10 years ago by Ryan J Ollos

In [13916]: extracting the discussion string to the realm attribute is a good idea. All IResourceManager and model classes in the Trac core should follow this pattern. Only a few do, such as the WikiPage model: t:browser:/tags/trac-1.0.1/trac/wiki/model.py?marks=33#L30, but even in that case you can see instances where self.realm isn't used to construct Resource objects: t:browser:/tags/trac-1.0.1/trac/wiki/model.py?marks=43#L30. I started working on fixing this issue in the Trac core a few weeks back but never got around to finishing. Maybe I will find time to pick that up again this weekend.

comment:9 Changed 10 years ago by Ryan J Ollos

forum_cols was added as an attribute of the DiscussionDb class, but the _get_forum method which references the forum_cols attribute via the call self.forum_cols was moved to DiscussionApi. It looks like that might result in an AttributeError.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:10 Changed 10 years ago by Steffen Hoffmann

In 13917:

DiscussionPlugin: Rework other single item getter methods, refs #11706.

While leaving single column references in api module looks like imperfect
isolation of schema-related information, it seems reasonable to at least
maintain table column lists in model module.

comment:11 in reply to:  9 Changed 10 years ago by Steffen Hoffmann

Replying to rjollos:

forum_cols was added as an attribute of the DiscussionDb class, but the _get_forum method which references the forum_cols attribute via the call self.forum_cols was moved to DiscussionApi. It looks like that might result in an AttributeError.

I've been uneasy about this too, however the class inheritance of DiscussionDb blends all attributes nicely to DiscussionApi, so I just did the same for other column name tuples as well.

comment:12 Changed 10 years ago by Ryan J Ollos

I see. I didn't see that the base class was DiscussionDb, in which case I don't see any problem with it.

comment:13 in reply to:  12 Changed 10 years ago by Steffen Hoffmann

Replying to rjollos:

I see. I didn't see that the base class was DiscussionDb, in which case I don't see any problem with it.

Thanks for code-review anyway. Being a bit suspicious doesn't hurt.

comment:14 Changed 10 years ago by Steffen Hoffmann

Replying to rjollos:

In [13916]: extracting the discussion string to the realm attribute is a good idea.

Interesting. While I did this change without knowing that pattern from other resource managers, mainly for reducing string repetitions, I see the point in making it a common class attribute.

comment:15 in reply to:  14 Changed 10 years ago by Ryan J Ollos

Replying to hasienda:

Replying to rjollos:

In [13916]: extracting the discussion string to the realm attribute is a good idea.

Interesting. While I did this change without knowing that pattern from other resource managers, mainly for reducing string repetitions, I see the point in making it a common class attribute.

I went ahead and made a ticket: t:#11609.

comment:16 Changed 10 years ago by Steffen Hoffmann

In 13919:

DiscussionPlugin: More PEP8 method cleanup, refs #11706.

Adding unit tests for reviewed methods we've reached 32 tests, all passed.

comment:17 Changed 10 years ago by Steffen Hoffmann

In 13920:

DiscussionPlugin: Review more complex methods, refs #11706.

Apart from usual PEP8 issues some SQL queries could be replaced with existing
method calls for robustness and saving lines in the module.
Remove debug logging, where resource changes in context are obvious anyway.

comment:18 Changed 10 years ago by Steffen Hoffmann

In 13921:

DiscussionPlugin: Stop requests lacking view permission to whole realm, refs #756 and #11706.

comment:19 Changed 10 years ago by Ryan J Ollos

Could you use context.req.perm.require('DISCUSSION_VIEW') in [13921]?: trac:browser:/tags/trac-0.11/trac/perm.py?marks=543#L543.

comment:20 in reply to:  19 Changed 10 years ago by Steffen Hoffmann

Replying to rjollos:

Could you use context.req.perm.require('DISCUSSION_VIEW') in [13921]?

You would prefer the explicit method call, wouldn’t you? No problem for me as both notations should be nearly equivalent, even technically IIRC.

comment:21 Changed 10 years ago by Steffen Hoffmann

In 13922:

DiscussionPlugin: Don't need to custom permission error, refs #756 and #11706.

This would be a reason to use the previous pattern. Thanks for the hint.

Version 0, edited 10 years ago by Steffen Hoffmann (next)

comment:22 Changed 10 years ago by Steffen Hoffmann

In [13923]:

DiscussionPlugin: Do not insert unescaped values into SQL query string.

This issue in model._get_items() went through on first review for [13919]. Luckily seeing the same thing done right in get_topics pointed it out. In a similar notion unregistered users are retrieved by set difference instead of iteration over topic['subscribers'] in _prepare_topic() too.

comment:23 Changed 10 years ago by Steffen Hoffmann

In 13925:

DiscussionPlugin: More PEP8 (excessive white-space, line-wrap), simplifications and style changes, refs #11706.

comment:24 Changed 10 years ago by Steffen Hoffmann

In 13926:

DiscussionPlugin: Consolidate remaining API methods for getting db content, refs #11706.

This includes a fix for api.get_users, that was broken in [13925].

comment:25 Changed 10 years ago by Steffen Hoffmann

In 13929:

DiscussionPlugin: Refactor and split methods into util modul functions, refs #11706.

49 tests document both, the code itself as well as review progress.

comment:26 Changed 10 years ago by Steffen Hoffmann

In 13930:

DiscussionPlugin: Prevent AttributeError when using API methods with pristine Trac context, refs #8637 and #11706.

This shall fix the issue reported for email2trac (yet untested) as well as
contribute to code robustness in general.

comment:27 Changed 10 years ago by Steffen Hoffmann

In 13935:

DiscussionPlugin: Complete db access API methods, refs #11706.

A missing import from previous modifications has been spotted on creation of
corresponding tests.

comment:28 Changed 10 years ago by Steffen Hoffmann

In 13936:

DiscussionPlugin: Reorganize search source code, refs #756 and #11706.

In fixing discussion resource links this is a follow-up to [13914].

The remains after moving db access code to model don't justify to keep
search as a separate module. Added search should help preventing further
regressions for this functionality.

comment:29 Changed 10 years ago by Steffen Hoffmann

In 13944:

DiscussionPlugin: Review wiki macros and syntax provider code, refs #756 and #11706.

This includes a wide range of changes from adding another db access method to
api over moving SQL into model module to correcting resource ID as
required for fine-grained permission support.

Adding the missing import from trac.resource as follow-up to [9640] suggests
rare use of discussion WikiMacros so far.

comment:30 Changed 10 years ago by Steffen Hoffmann

In 13945:

DiscussionPlugin: Extend use of new attribute getter api method, refs #11706.

Preventing TypeError in case of invalid ID argument is required for this
use, and doesn't hurt for others anyway.

comment:31 Changed 8 years ago by Ryan J Ollos

In 15548:

0.10dev: Pass format_to_oneliner_no_links in template data

Refs #11706.

comment:32 Changed 8 years ago by Ryan J Ollos

In 15549:

0.10dev: Fix typo in [15548]

Refs #11706, #12762.

comment:33 Changed 7 years ago by Ryan J Ollos

In 16012:

0.10dev: Fix incorrect string formatting and comparisons, missing imports

Refs #11706.

comment:34 Changed 7 years ago by Ryan J Ollos

In 16013:

1.2dev: Fix incorrect string formatting and comparisons, missing imports

Merge of r16012 from 1.0 branch.

Refs #11706, #12971.

comment:35 Changed 7 years ago by Ryan J Ollos

In 16014:

0.10.0: Bump version from 0.10dev

Refs #11706.

comment:36 Changed 7 years ago by Ryan J Ollos

Going forward only major fixes will be applied to the 1.0 branch. Code review and improvements will continue in #12971.

comment:37 Changed 7 years ago by Ryan J Ollos

Owner: Steffen Hoffmann deleted

comment:38 Changed 7 years ago by Ryan J Ollos

Resolution: fixed
Status: newclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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

 
Note: See TracTickets for help on using tickets.