Last Comment Bug 785051 - Display tree 'motd' field after 'reason', once added to TreeStatus
: Display tree 'motd' field after 'reason', once added to TreeStatus
Status: RESOLVED FIXED
[sheriff-want]
:
Product: Tree Management Graveyard
Classification: Graveyard
Component: TBPL (show other bugs)
: Trunk
: All All
: -- normal
: ---
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
Depends on: 790559
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-23 06:15 PDT by Ed Morley [:emorley]
Modified: 2015-04-13 15:57 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.20 KB, patch)
2012-08-31 03:05 PDT, Graeme McCutcheon [:graememcc]
emorley: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2012-08-23 06:15:28 PDT
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).
Comment 1 Graeme McCutcheon [:graememcc] 2012-08-31 03:05:33 PDT
Created attachment 657203 [details] [diff] [review]
v1

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.
Comment 2 Ed Morley [:emorley] 2012-09-03 06:29:24 PDT
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.
Comment 3 Ed Morley [:emorley] 2012-09-03 06:29:43 PDT
(And sorry for the delay, was away on Friday).
Comment 4 Ed Morley [:emorley] 2012-09-03 06:33:00 PDT
And thank you for doing this :-D
Comment 6 Ed Morley [:emorley] 2012-09-13 14:17:19 PDT
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) :-)
Comment 7 Ed Morley [:emorley] 2012-09-13 14:24:30 PDT
(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 :-)

Note You need to log in before you can comment on or make changes to this bug.