Eliminate redundant code in milestone and admin milestone templates / modules

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Jun Omae]

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="...">.

[Ryan J Ollos]

One additional minor styling change in [12875].

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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:

[Jun Omae]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

Replying to jomae:

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

Fixed in [12932].

Additional changes in

• [12934], merged in [12935]
• [12936], merged in [12938]
• [12941]

Message extractions in [12933,12937,12939].

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

Screen captures showing the latest:

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

Admin Version and Component Edit pages

Milestone Edit page

[Ryan J Ollos]

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

[Ryan J Ollos]

Changes committed to trunk in [12964:12967].

[Ryan J Ollos]

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.

[Ryan J Ollos]

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