Closed Bug 785051 Opened 12 years ago Closed 12 years ago

Display tree 'motd' field after 'reason', once added to TreeStatus

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emorley, Assigned: graememcc)

References

Details

(Whiteboard: [sheriff-want])

Attachments

(1 file)

At the moment, we stuff the treestatus reason field full of generic tree messages / MOTD type strings.

I've filed:
https://github.com/catlee/treestatus/issues/24
...to add a new motd/message type field to TreeStatus, so people don't accidentally overwrite the MOTD when closing the tree/adding a reason (as happened last night).

Once the new field has been added, TBPL will need to concatenate reason+motd at:
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/3fe5fa1631bc/js/UserInterface.js#l184

I guess there's nothing stopping us from moving the MOTD parts/formatting them differently/even maybe not showing them if the tree is CLOSED (to prevent the situations where we end up with 3 million lines of text due to a long closure message).
Attached patch v1Splinter Review
As suggested, if the tree is closed, it will display status+reason, and if open status + reason + motd.

It's also gated on the presence of the motd field in the JSON returned from treestatus. If that field isn't present (or empty) it will display status+reason. In particular, this avoids having to co-ordinate pushing this patch with the treestatus landing - it can be deployed immediately with no visible change in behaviour, and will automatically do the right thing when the treestatus.m.o change is deployed. We can then circle back later and remove the "&& data.message_of_the_day" part of the condition.
Attachment #657203 - Flags: review?(bmo)
Comment on attachment 657203 [details] [diff] [review]
v1

r=me, but I think we should move the:
+          if (data.reason)
+            baseMessage += '. ';

before the cssStatus == "open" conditional, so that the end of sentence punctuation doesn't vary depending on whether the tree is open or closed. This will also make the if-else clearer.

+        } else
+          $('#tree-status').html(baseMessage);
Please can we go for braces on both parts, or if we move the |if (data.reason)| per above, then we could have both without. Just not a huge fan of mix and match in the same if-else.
Attachment #657203 - Flags: review?(bmo) → review+
(And sorry for the delay, was away on Friday).
And thank you for doing this :-D
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/03803dbdf1ee
Assignee: nobody → graememcc_firefox
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Missed getting marked as depending on bug 790559, since there was bug number in the commit.

Anyway, this is in production now (and has been for a day or so) :-)
Depends on: 790559
(In reply to Ed Morley [:edmorley UTC+1] from comment #6)
> Missed getting marked as depending on bug 790559, since there was bug number
> in the commit.

no bug number, even :-)
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: