Edgewall Software
Modify

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#11565 closed task (fixed)

Reduce code duplication in about and error pages

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: general Version: 1.0-stable
Severity: normal Keywords: about error refactoring
Cc:
Release Notes:

Reduced code duplication in the about.html and error.html templates.

API Changes:
  • New template environment_info.html renders the System Information, Installed Plugins and Configuration Information.
  • Added get_config_info method to the Environment class.
  • Merged the about.css stylesheet into trac.css.

Description (last modified by Ryan J Ollos)

Having to make modifications in both error.html and about.html in #11558, I was looking at the duplicated code and possibilities for re-use.

The major change that will be proposed is the extraction of code to a new template environment_info.html, which is included in both error.html and about.html.

The differences I could see between the information displayed on the two pages and the steps I tool to resolve the differences are:

  1. Headings on tables in about.html, but no headings in error.html - environment_info.html has heading on tables.
  2. Plugins table on about.html also shows disabled plugins - existing behavior preserved on both pages.
  3. Plugins table on error.html links to frames in stacktrace - existing behavior preserved on both pages.
  4. Markup was slightly different, with divs enclosing each table on about.html, and the ids on the divs rather than on the tables, as error.html does - tables in environment_info.html now have enclosing divs.
  5. The plugins section on the error page read Enabled Plugins; on the about page it read Installed Plugins. The environment_info.html template reads Installed Plugins, however the error.html page continues to show only enabled plugins, whereas the about.html page shows both Enabled and Disabled plugins.

The change in markup on error.html in (4) means that these changes should only be made on the trunk. These changes will also help with #11548, in reducing the number of code changes needed for adding templates to the environment info on about.html and error.html (at least on the trunk).

The CSS for #content.error and #content.about was very similar. I merged about.css into trac.css, and merged several of the rules so that they apply to the new template environment_info.html.

Attachments (0)

Change History (18)

comment:1 Changed 4 years ago by Ryan J Ollos

I'll wait until the changes in #11558 are committed and rebase my local t11565 branch on those changes before posting log:rjollos.git:t11565.

comment:2 Changed 4 years ago by Ryan J Ollos

Description: modified (diff)

comment:3 Changed 4 years ago by Ryan J Ollos

Keywords: refactoring added
Milestone: undecided1.1.2
Owner: set to Ryan J Ollos
Status: newassigned
Version: 1.0-stable

comment:4 Changed 4 years ago by Ryan J Ollos

When the about.py module was added in [4264] there was a page for each section: /about/config and /about/plugins. In [4593] the sections were combined into a single page. The regex could be simplified after [4593]:

-re.match(r'/about(?:_trac)?(?:/.+)?$', req.path_info)
+re.match(r'/about(?:_trac)?$', req.path_info)

Is there any reason to not make this change on the trunk?

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

comment:5 Changed 4 years ago by Ryan J Ollos

Description: modified (diff)

Proposed changes can be found in log:rjollos.git:t11565.

comment:6 Changed 4 years ago by Ryan J Ollos

Change from comment:4 committed to trunk in [12649].

comment:7 Changed 4 years ago by Ryan J Ollos

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Refactoring of about.html and error.html committed in [12650,12652].

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

comment:8 Changed 4 years ago by Ryan J Ollos

API Changes: modified (diff)

comment:9 Changed 4 years ago by Jun Omae

Sorry for late.

I don't think that we should add system_info decorated with @property because Environment class has systeminfo property. That's confusing.

comment:10 Changed 4 years ago by Ryan J Ollos

I didn't notice. It's as confusing as having get_systeminfo and get_system_info. systeminfo could be changed to _systeminfo.

comment:11 in reply to:  10 ; Changed 4 years ago by Ryan J Ollos

Replying to rjollos:

systeminfo could be changed to _systeminfo.

Nevermind, that doesn't make sense. I'll look more closely.

comment:12 Changed 4 years ago by Ryan J Ollos

Resolution: fixed
Status: closedreopened

comment:13 in reply to:  11 ; Changed 4 years ago by Jun Omae

Nevermind, that doesn't make sense. I'll look more closely.

Okay.

Also, I noticed that Trac core and some plugins in trac-hacks directly append entries to env.systeminfo….

$ grep -rF '.systeminfo' trac tracopt
trac/test.py:        self.systeminfo = []
trac/env.py:        self.systeminfo = []
trac/env.py:        info = self.systeminfo[:]
trac/db/mysql_backend.py:            self.env.systeminfo.extend([('MySQL', mysql_info),
trac/db/sqlite_backend.py:            self.env.systeminfo.extend([('SQLite', sqlite_version_string),
trac/db/postgres_backend.py:            self.env.systeminfo.append(('psycopg2', self._version))
trac/tests/env.py:        self.systeminfo = []
trac/web/main.py:                env.systeminfo.append((env.webfrontend,
tracopt/versioncontrol/git/git_fs.py:            self.env.systeminfo.append(('GIT', self._version['v_str']))
$ grep -rF --include='*.py' '.systeminfo' .
./fullblogplugin/0.11/tracfullblog/core.py:        self.env.systeminfo.append(('FullBlog',
./customfieldadminplugin/0.11/customfieldadmin/tests/api.py:                    in self.env.systeminfo)
./customfieldadminplugin/0.11/customfieldadmin/api.py:            self.env.systeminfo.append(('CustomFieldAdmin', __version__))
./xmlrpcplugin/trunk/tracrpc/api.py:        self.env.systeminfo.append(('RPC',
./gitplugin/0.11/tracext/git/git_fs.py:                        self.env.systeminfo.append(('GIT', self._version['v_str']))
./gitplugin/0.12/tracext/git/git_fs.py:                        self.env.systeminfo.append(('GIT', self._version['v_str']))
./bittenforgitplugin/0.11/0.6b2/bitten/master.py:        self.env.systeminfo.append(('Bitten',

comment:14 Changed 4 years ago by Ryan J Ollos

Python 2.5 support has been officially dropped on the trunk: TracDev/ApiChanges/1.1.1, so we can use the setter attribute.

These changes might work, but I'll need to do more testing tomorrow: log:rjollos.git:t11565.3. After the change accessing Environment.systeminfo will return a list with more entries than previously. I'm not sure there's a perfect solution that leave the API completely unchanged, but it doesn't look like anyone is doing a get on systeminfo, so it seems like a fairly safe change. The other solution, a partial revert of [12650] leaves us with the confusing get_systeminfo and get_system_info, which is not good.

comment:15 Changed 4 years ago by Ryan J Ollos

API Changes: modified (diff)
Resolution: fixed
Status: reopenedclosed

Addition of systeminfo property reverted in [12653].

comment:16 in reply to:  13 ; Changed 4 years ago by Ryan J Ollos

Replying to jomae:

Also, I noticed that Trac core and some plugins in trac-hacks directly append entries to env.systeminfo….

It seems like it would be cleaner if we used ISystemInfoProvider everywhere. Care would need to be taken for issues like that noted in comment:5:ticket:8908. I might explore whether some refactoring is possible.

comment:17 in reply to:  16 Changed 4 years ago by Ryan J Ollos

Replying to rjollos:

It seems like it would be cleaner if we used ISystemInfoProvider everywhere. Care would need to be taken for issues like that noted in comment:5:ticket:8908. I might explore whether some refactoring is possible.

These changes were made in #11587.

comment:18 Changed 3 years ago by Ryan J Ollos

Related changes proposed in comment:34:ticket:11901.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.