Questions about licensing and some suggested refactoring

[Ryan J Ollos]

May I suggest a few changes (which I'm happy to push myself given approval from the author)?

• License:
  • Clarify which of the GPL licenses is used. Better yet, would you consider changing the license of this plugin and your other plugins (PrivateCommentPlugin and HideableQueryPlugin) to BSD 3-Clause, which is what the Trac core uses, and what I consider to be the most "friendly" license for Trac development? :)
  • Add text of the license in a COPYING file.
  • Add license headers to each file.
• Code:
  • Change tabs to spaces source files (per the ​PEP-008 recommendation). The template contains a mix of tabs and spaces.
  • As in [12959], we don't have to close the cursor, and as in [12958] it isn't necessary to print the SQL statements to the debug log since the Trac API handles this.
  • As in [12960], we can get the report id from the request args and simplify the code in pre_process_request. reg.args.get('id') will return the ID number when on the /report/id page, and will return None when on the /report page. Actually, you do something very similar in render_admin_panel.
  • Allow a user with REPORT_ADMIN and PERMISSION_ADMIN to grant/revoke permissions for a report, as opposed to requiring TRAC_ADMIN.

I haven't thought about this for too long, so maybe I'm overlooking the obvious, but what is the use-case for having admin-defined custom permissions for each report as opposed to just generating a VIEW permission for each report that exists REPORT_VIEW_1, REPORT_VIEW_2, ...? Making this change could eliminate the need for the admin panel. I don't think we'd even need a database table, since all of the permissions could be generated in IPermissionRequestion.get_permission_actions, given the existing report numbers.

Actually, all of this can be done anyway with ​t:TracFineGrainedPermissions (the accuracy of that statement should be checked given the existence of defects such as ​t:#11069), so maybe the plugin should just be a nicer interface to editing an authz file for reports? There is currently a FineGrainedPageAuthzEditorPlugin that we plan to integrate into AccountManagerPlugin in #9947. A friendly interface to the authz file could allow a user to make specific reports private without requiring a specialized plugin.

[Ryan J Ollos]

If/while implementing these changes, we'll want to compare against the changes made in [12958:12960] for the PrivateCommentPlugin. I should have done a better job of separating out the tabs -> spaces changes into a single changeset that could be ignored for the purpose of code review.

[Michael Henke]

You may implement the things mentioned. Also I'm ok with the BSD license. At the moment I don't really have time for trac so having another maintainer would be nice. The reason why I made this is that I didn't find a simple way to hide reports at that point in time. The plugin you mentioned looks good by the way.

[Ryan J Ollos]

(In [13033]) Refs #11049: Replaced tabs with spaces in source code.

[Ryan J Ollos]

(In [13034]) Refs #11049: PEP-008 and other changes to conform to Trac coding conventions.

[Ryan J Ollos]

(In [13035]) Refs #11049: Changed category to "Reports".

[Ryan J Ollos]

(In [13036]) Refs #11049:

• Changed license to BSD 3-Clause with permission of author.
• Added license header to source code files.
• Added text of license in COPYING file.

[Ryan J Ollos]

(In [13037]) Refs #11049: Part of [13036].

[Ryan J Ollos]

(In [13038]) Refs #11049: Modified entry point and changed plugin name to be CamelCase.

[Ryan J Ollos]

(In [13039]) Refs #11049:

• Removed instances of cursor.close(). It isn't necessary to close the cursor.
• Removed DEBUG level logging of SQL statements. Debug level logging of SQL statements is implicit in the Trac database API and can be turned on by setting [trac] debug_sql = True.

[Ryan J Ollos]

(In [13040]) Refs #11049: Simplified implementation of pre_process_request.

[Ryan J Ollos]

Replying to rjollos:

Actually, all of this can be done anyway with ​t:TracFineGrainedPermissions (the accuracy of that statement should be checked given the existence of defects such as ​t:#11069), so maybe the plugin should just be a nicer interface to editing an authz file for reports? There is currently a FineGrainedPageAuthzEditorPlugin that we plan to integrate into AccountManagerPlugin in #9947. A friendly interface to the authz file could allow a user to make specific reports private without requiring a specialized plugin.

I've captured these ideas here.

[Ryan J Ollos]

(In [13042]) Refs #11049: Renamed 0.12 directory to trunk since plugin now works with Trac 1.0 as well as 0.12. Added an empty tags directory.

[Ryan J Ollos]

(In [13043]) Refs #11049: Tagging version 0.3 of plugin.

[Ryan J Ollos]

(In [13044]) Refs #11049: Tagging version 0.2 of plugin.

[Ryan J Ollos]

(In [13045]) Refs #11049: Tagging version 0.1 of plugin.

[Ryan J Ollos]

(In [13046]) Refs #11049: Part of [13038]. Entry point was incorrect.

[Ryan J Ollos]

Screen capture on project wiki page has been updated to reflect changes in this ticket: wiki:PrivateReportsPlugin:PrivateReportAdmin-Trac1.0.png​

[Ryan J Ollos]

(In [13050]) Fixes #11052, Refs #11049: (PrivateCommentPlugin)

• Changed license to BSD 3-Clause with permission of author.
• Changed plugin name to CamelCase.