394248 - Add some readability to the flag mail template

Status: NEW

(Bugzilla :: Attachments & Requests, defect)

Description

[Marc Schumann [:Wurblzap]]

Attachment: Patch

As it is, I think the flag mail template is very confusing. It's tied tightly to English grammar, and it's rather difficult to follow its code paths and magic variables. This gives trouble to code contributors and localizers.

Attached is a first attempt to remedy this.

Comment 1

[Frédéric Buclin]

I don't like these "own request" strings. This makes the logic so complicated. That's mostly a r- due to that. Most strings can be merged two by two.

Comment 2

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2

Ok, this is what both of us can live with probably.

Comment 3

[Frédéric Buclin]

Comment on attachment 280255 [details] [diff] [review]
Patch 2

>+[% IF flag.status == '?' %]
>+  [%-# We're asking for somebody else's decision. %]
>+  [% IF ! flag.setter %]
>+    [%-# This question hasn't been asked before. %]

A flag always has a setter, so the IF test is wrong. It seems you meant [% IF flag.setter.id == user.id %], i.e. you want to know if you are reassigning the request or not (per what is written in the ELSE block).


>+    [% IF flag.requestee %]
>+      [%-# We're asking specifically. %]
>+      [% user.identity %] has [% body_activities.${flag.status} %] [%+ flag.requestee.identity %] for [% flag.type.name %]:
>+    [% ELSE %]
>+      [%-# We're asking the wind. %]
>+      [% user.identity %] has [% body_activities.${flag.status} %] for [% flag.type.name %]:
>+    [% END %]

All this code do is to remove the extra whitespace if no requestee is defined. I'm not sure it worths the ELSE block.



>+  [% ELSE %]
>+    [%-# This question has been asked before (reassignment). There is no
>+       # immediate need to special-case this, but it's friendlier to have a
>+       # dedicated message.
>+       #%]

Huh? Unless I miss something, this comment has nothing to do here.


>+    [%-# We're reassigning somebody else's request. %]
>+    [% user.identity %] has reassigned [% flag.setter.identity %]'s request for [% flag.type.name %] to [% flag.requestee.identity %]:

This doesn't work if there is no new requestee (request from the wind) as the sentence would end as "to ".


The rest of the code looks correct.

Comment 4

[Frédéric Buclin]

Too late for 3.2.

Comment 5

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 6

[Marc Schumann [:Wurblzap]]

5 years down the road, this is WONTFIX as far as I'm concerned.