Modify

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#11049 closed enhancement (fixed)

Questions about licensing and some suggested refactoring

Reported by: rjollos Owned by: rjollos
Priority: normal Component: PrivateReportsPlugin
Severity: normal Keywords: license refactoring
Cc: Trac Release:

Description

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.

Attachments (0)

Change History (21)

comment:1 Changed 20 months ago by rjollos

  • Keywords license refactoring added

comment:2 Changed 20 months ago by rjollos

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.

comment:3 Changed 20 months ago by mhenke

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.

comment:4 Changed 20 months ago by rjollos

  • Owner changed from mhenke to rjollos
  • Status changed from new to assigned

comment:5 Changed 20 months ago by rjollos

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

comment:6 Changed 20 months ago by rjollos

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

comment:7 Changed 20 months ago by rjollos

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

comment:8 Changed 20 months ago by rjollos

(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.

comment:9 Changed 20 months ago by rjollos

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

comment:10 Changed 20 months ago by rjollos

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

comment:11 Changed 20 months ago by rjollos

(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.

comment:12 Changed 20 months ago by rjollos

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

comment:13 in reply to: ↑ description Changed 20 months ago by rjollos

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.

comment:14 Changed 20 months ago by rjollos

(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.

comment:15 Changed 20 months ago by rjollos

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

comment:16 Changed 20 months ago by rjollos

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

comment:17 Changed 20 months ago by rjollos

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

comment:18 Changed 20 months ago by rjollos

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

comment:19 Changed 20 months ago by rjollos

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

comment:20 Changed 20 months ago by rjollos

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

comment:21 Changed 20 months ago by rjollos

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

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

Add Comment

Modify Ticket

Action
as closed The owner will remain rjollos.
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.