Modify

Opened 6 months ago

Last modified 5 months ago

#11706 new task

Code review

Reported by: hasienda Owned by: hasienda
Priority: high Component: DiscussionPlugin
Severity: normal Keywords:
Cc: rjollos 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 (30)

comment:1 Changed 6 months ago by hasienda

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

In 13891:

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

comment:3 Changed 6 months ago by hasienda

In 13893:

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

comment:4 Changed 6 months ago by hasienda

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

In 13910:

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

comment:6 Changed 6 months ago by hasienda

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

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

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

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

Version 0, edited 6 months ago by rjollos (next)

comment:10 Changed 6 months ago by hasienda

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

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

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

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 follow-up: Changed 6 months ago by 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.

comment:15 in reply to: ↑ 14 Changed 6 months ago by rjollos

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

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

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

In 13921:

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

comment:19 follow-up: Changed 6 months ago by rjollos

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

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

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.

Last edited 6 months ago by hasienda (previous) (diff)

comment:22 Changed 6 months ago by hasienda

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

In 13925:

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

comment:24 Changed 6 months ago by hasienda

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

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

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

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

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

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

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.

Add Comment

Modify Ticket

Action
as new 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.