Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#11499 closed enhancement (fixed)

Eliminate redundant code in milestone and admin milestone templates / modules

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: ticket system Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
  • Changed styling of Milestone, Ticket System Admin and Repository Admin edit pages by removing fieldsets around the Descriptions.
  • When milestone edit validations fail on the Milestone Admin page, a warning is added rather than redirecting to the error page.
API Changes:

Added methods to MilestoneModule class:

  • get_default_due
  • save_milestone

Added function get_num_tickets_for_milestone (since 1.2; the similar method Milestone.get_num_tickets was added in 1.1.2, but replaced with get_num_tickets_for_milestone in 1.2).

New template milestone_edit_form.html for editing a milestone is used on the milestone_edit.html and admin_milestone.html pages. The MilestoneModule is used in trac.ticket.admin.MilestoneAdminPanel to eliminate redundant code.

Description

An important step towards #3754 (see comment:7:ticket:3754) is to merge portions of the milestone_edit.html and admin_milestones.html pages. It may be possible to eliminate some redundant code in trac.ticket.admin and trac.ticket.roadmap as well.

Attachments (13)

AdminMilestoneEdit.png (37.9 KB ) - added by Ryan J Ollos 5 years ago.
RoadmapMilestoneEdit.png (43.2 KB ) - added by Ryan J Ollos 5 years ago.
CreateReport.png (30.3 KB ) - added by Ryan J Ollos 5 years ago.
EditReport.png (72.1 KB ) - added by Ryan J Ollos 5 years ago.
RetargetingNoTickets.png (37.8 KB ) - added by Ryan J Ollos 5 years ago.
RetargetingFieldset.png (43.3 KB ) - added by Ryan J Ollos 5 years ago.
AdminEditMilestone_NoMilestoneModify_20140714.png (28.6 KB ) - added by Ryan J Ollos 5 years ago.
AdminEditMilestone_20140714.png (27.6 KB ) - added by Ryan J Ollos 5 years ago.
AdminEditVersions_20140714.png (22.2 KB ) - added by Ryan J Ollos 5 years ago.
AdminEditComponent_20140714.png (20.9 KB ) - added by Ryan J Ollos 5 years ago.
EditMilestone_DueCompleted_20140714.png (38.4 KB ) - added by Ryan J Ollos 5 years ago.
EditMilestone_20140714.png (37.0 KB ) - added by Ryan J Ollos 5 years ago.
EditMilestone_NoTickets_20140714.png (32.1 KB ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 by Ryan J Ollos, 6 years ago

Owner: set to Ryan J Ollos
Status: newassigned

Changes to templates can be found in log:rjollos.git:t11499. I'm still working on changes to the request handling, which are a bit more complex, and then more tests need to be added.

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

comment:2 by Ryan J Ollos, 5 years ago

I committed the changes in comment:1 in [12858:12860]. I'll proceed by looking at what code we can reuse from MilestoneModule in MilestoneAdminPanel.

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

comment:3 by Ryan J Ollos, 5 years ago

Milestone: 1.1.31.1.2

There are a few minor defects in [12858]. I'll fix those and add more functional tests.

comment:4 by Ryan J Ollos, 5 years ago

The issues with [12858] are:

  • The textarea is not readonly when user doesn't have MILESTONE_MODIFY.
  • The submit button is not disabled when user doesn't have MILESTONE_MODIFY.

One other small detail: I can't see a need for both the hidden form field id trunk/trac/ticket/templates/milestone_edit.html@12857:58#L49 and parsing the id from the path: trunk/trac/ticket/roadmap.py@12857:658#L652. The field in the template has been there since [600#file1]. The parsing of id has existed since match_request was implemented in [1586#file14]. It seems like one of the two could be removed.

comment:5 by Ryan J Ollos, 5 years ago

Issues described in comment:4 fixed in [12870]. Improvements to template and styling in [12869,12871:12872].

in reply to:  description comment:6 by Ryan J Ollos, 5 years ago

Replying to rjollos:

It may be possible to eliminate some redundant code in trac.ticket.admin and trac.ticket.roadmap as well.

Extensive changes to trac.ticket.admin and trac.ticket.roadmap in log:rjollos.git:t11499.3. The changes eliminate redundant code and should ease the implementation of #3754.

On the admin milestone page, TracErrors raised on POST have been replaced with a warning and a redirect back to the edit page.

comment:7 by Jun Omae, 5 years ago

We should provide i18n/l10n message in the legend element of [12781].

-      <legend py:if="compact" py:content="'Modify Milestone:' if can_modify else 'View Milestone:'"></legend>
+      <legend py:if="compact" py:choose="">
+        <py:when test="can_modify" i18n:msg="">Modify Milestone:</py:when>
+        <py:otherwise i18n:msg="">View Milestone:</py:otherwise>
+      </legend>

BTW, <fieldset py:strip="not compact">...<legend py:if="compact">, it doesn't seem to be elegant…. I think we could use <py:def function="...">.

comment:8 by Ryan J Ollos, 5 years ago

One additional minor styling change in [12875].

I'll look into the suggestions from comment:7 shortly.

in reply to:  7 ; comment:9 by Ryan J Ollos, 5 years ago

Replying to jomae:

BTW, <fieldset py:strip="not compact">...<legend py:if="compact">, it doesn't seem to be elegant…. I think we could use <py:def function="...">.

Yeah, it's not very elegant or maintainable. Initially I was trying to keep the templates unchanged for a pure refactoring. However, it might make sense to change the layout and styling a little bit. I've eliminated the compact variable now:

I have a few more tweaks to make and then I'll post changes for review. The screen captures above are just to illustrate the basic idea.

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

by Ryan J Ollos, 5 years ago

Attachment: AdminMilestoneEdit.png added

by Ryan J Ollos, 5 years ago

Attachment: RoadmapMilestoneEdit.png added

by Ryan J Ollos, 5 years ago

Attachment: CreateReport.png added

by Ryan J Ollos, 5 years ago

Attachment: EditReport.png added

by Ryan J Ollos, 5 years ago

Attachment: RetargetingNoTickets.png added

by Ryan J Ollos, 5 years ago

Attachment: RetargetingFieldset.png added

comment:10 by Ryan J Ollos, 5 years ago

Some additional changes:

  • Of the existing forms in Trac for creating resources, the Report Edit form is the only one that does not contain a fieldset. The following changes are proposed proposed for 1.0-stable. The legend content is Create Report: when the report does not exist, and Modify Report: when the report exists. Note that the submit buttons are located below the fieldset.

  • The Submit buttons on the Wiki Edit and New Ticket pages are located below the fieldset. This change was made to the Milestone Edit form and the edit forms for the other ticket system objects.
  • Added a fielset around the retargeting elements:

in reply to:  9 ; comment:11 by Jun Omae, 5 years ago

Replying to rjollos:

Replying to jomae:

BTW, <fieldset py:strip="not compact">...<legend py:if="compact">, it doesn't seem to be elegant…. I think we could use <py:def function="...">.

Yeah, it's not very elegant or maintainable. Initially I was trying to keep the templates unchanged for a pure refactoring.

PoC for use of <py:def function=...> is like this. It's a bit maintainable, but not elegant.

  <py:def function="_top">
    <div class="field">
      <input type="hidden" name="id" value="$milestone.name" />
      <input type="hidden" name="action" value="edit" />
      <label>Name:<br />
        <input type="text" id="name" name="name" class="trac-autofocus" size="32"
               value="${milestone.name or req.args.get('name')}" readonly="$readonly" />
      </label>
    </div>
  </py:def>
  <py:def function="_middle">...</py:def>
  <py:def function="_bottom">...</py:def>
  <form id="edit" class="mod" method="post" action=""
        py:with="disabled = 'disabled' if can_modify is False else None;
                 readonly = 'readonly' if can_modify is False else None"
        py:choose="">
    <fieldset py:when="compact">
      <legend py:choose="">
        <py:when test="can_modify" i18n:msg="">Modify Milestone:</py:when>
        <py:otherwise i18n:msg="">View Milestone:</py:otherwise>
      </legend>
      ${_top()}
      ${_middle()}
      ${_bottom()}
    </fieldset>
    <py:otherwise>
      ${_top()}
      <fieldset>
        <legend>Schedule</legend>
        ${_middle()}
      </fieldset>
      ${_bottom()}
    </py:otherwise>
  </form>
</html>

However, it might make sense to change the layout and styling a little bit. I've eliminated the compact variable now: […]

The changes are reasonable and these screenshots looks good to me. It might be good without the Schedule fieldset and legend elements because nested fieldset elements are verbose.

Replying to rjollos:

  • The Submit buttons on the Wiki Edit and New Ticket pages are located below the fieldset. This change was made to the Milestone Edit form and the edit forms for the other ticket system objects.

Agreed. I think that's good styling to locate the buttons outside the fieldset.

comment:12 by Ryan J Ollos, 5 years ago

Thanks. I'll experiment without the Schedule fieldset. When you say the nested fieldsets are verbose, do you mean the page looks cluttered, or are you referring to accessibility (e.g. how a screen reader would act on the content)?

I created #11664 for the changes to the Report Edit page.

in reply to:  11 comment:13 by Ryan J Ollos, 5 years ago

Replying to jomae:

Replying to rjollos:

  • The Submit buttons on the Wiki Edit and New Ticket pages are located below the fieldset. This change was made to the Milestone Edit form and the edit forms for the other ticket system objects.

Agreed. I think that's good styling to locate the buttons outside the fieldset.

I created #11666 to apply the change to 1.0-stable. Although on second thought it may be better to apply the change to the trunk since it could cause problems for a plugin XPath selector in an ITemplateStreamFilter.

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

in reply to:  7 comment:14 by Ryan J Ollos, 5 years ago

Replying to jomae:

We should provide i18n/l10n message in the legend element of [12781].

Fixed in [12932].

Additional changes in

Message extractions in [12933,12937,12939].

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

in reply to:  4 comment:15 by Ryan J Ollos, 5 years ago

Replying to rjollos:

One other small detail: I can't see a need for both the hidden form field id trunk/trac/ticket/templates/milestone_edit.html@12857:58#L49 and parsing the id from the path: trunk/trac/ticket/roadmap.py@12857:658#L652. The field in the template has been there since [600#file1]. The parsing of id has existed since match_request was implemented in [1586#file14]. It seems like one of the two could be removed.

The hidden field was removed in [12940].

comment:16 by Ryan J Ollos, 5 years ago

Some proposed changes in log:rjollos.git:t11499.5:

  • Removes the Schedule fieldset.
  • Removes the fieldset around the Description on several pages.
  • Adds a fieldset around the Retarget elements.

I still need to tweak the CSS a bit. The margins around the Description textarea and label are not good now.

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

by Ryan J Ollos, 5 years ago

by Ryan J Ollos, 5 years ago

by Ryan J Ollos, 5 years ago

by Ryan J Ollos, 5 years ago

by Ryan J Ollos, 5 years ago

Attachment: EditMilestone_20140714.png added

by Ryan J Ollos, 5 years ago

comment:17 by Ryan J Ollos, 5 years ago

Screen captures showing the latest:

Admin Milestone Edit page (with and w/o MILESTONE_MODIFY)

Admin Version and Component Edit pages

Milestone Edit page

comment:18 by Ryan J Ollos, 5 years ago

The changes in log:rjollos.git:t11499.6 implement what is shown in the screen capture above.

comment:19 by Ryan J Ollos, 5 years ago

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

Changes committed to trunk in [12964:12967].

comment:20 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

comment:21 by Ryan J Ollos, 3 years ago

I'm considering that we should not have coupling between ticket and milestone models. Changing the method Milestone.get_num_tickets added in [12964#file1], and which was modified in [14193#file0], to a function would reduce the coupling. The function would also be more consistent with similar functions, such as: trunk/trac/ticket/roadmap.py@15010:295#L295.

Changes for consideration: log:rjollos.git:t11499_reduce_coupling.

comment:22 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)

comment:21 changes committed in r15078, merged to trunk in r15079.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed 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.