Modify

Opened 15 months ago

Closed 15 months ago

Last modified 15 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 15 months ago by rjollos

  • Keywords license refactoring added

comment:2 Changed 15 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 15 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 15 months ago by rjollos

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

comment:5 Changed 15 months ago by rjollos

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

comment:6 Changed 15 months ago by rjollos

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

comment:7 Changed 15 months ago by rjollos

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

comment:8 Changed 15 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 15 months ago by rjollos

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

comment:10 Changed 15 months ago by rjollos

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

comment:11 Changed 15 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 15 months ago by rjollos

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

comment:13 in reply to: ↑ description Changed 15 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 15 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 15 months ago by rjollos

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

comment:16 Changed 15 months ago by rjollos

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

comment:17 Changed 15 months ago by rjollos

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

comment:18 Changed 15 months ago by rjollos

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

comment:19 Changed 15 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 15 months ago by rjollos

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

comment:21 Changed 15 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 .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from rjollos. Next status will be '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.