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



Tree Management Graveyard
5 years ago
2 years ago


(Reporter: emorley, Assigned: graememcc)



(Whiteboard: [sheriff-want])


(1 attachment)



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

I've filed:
...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:

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

5 years ago
Created attachment 657203 [details] [diff] [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 2

5 years ago
Comment on attachment 657203 [details] [diff] [review]

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+

Comment 3

5 years ago
(And sorry for the delay, was away on Friday).

Comment 4

5 years ago
And thank you for doing this :-D

Comment 5

5 years ago
Assignee: nobody → graememcc_firefox
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 6

5 years ago
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

Comment 7

5 years ago
(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.