Convert storage for Herald repetition policy to "text32"
ClosedPublic
Actions

Authored by epriestley on Jan 25 2018, 4:28 AM.

Details

Reviewers

Maniphest Tasks

T6203: Adjust nullability of various questionable columns
T13048: Implement a "take action only when this rule didn't match last time" repetition policy for Herald

Commits

rP204d1de68376: Convert storage for Herald repetition policy to "text32"

Summary

Depends on D18926. Ref T6203. Ref T13048. Herald rule repetition policies are stored as integers but treated as strings in most contexts.

After D18926, the integer stuff is almost totally hidden inside HeraldRule and getting rid of it completely isn't too tricky.

Do so now.

Test Plan

Created "only the first time" and "every time" rules. Did a SELECT on their rows in the database.
Ran migrations, got a clean bill of health from storage adjust.
Did another SELECT on the rows, saw a faithful conversion to strings "every" and "first".
Edited and reviewed rules, swapping them between "every" and "first".

Diff Detail

Repository

rP Phabricator

Branch

herald4

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 19186
Build 25914: Run Core Tests
Build 25913: arc lint + arc unit

Event Timeline

Rename "herlad" migration to "herald".

Harbormaster completed remote builds in B19176: Diff 45395.Jan 25 2018, 4:30 AM

Harbormaster completed remote builds in B19177: Diff 45396.

epriestley requested review of this revision.Jan 25 2018, 4:30 AM

epriestley added a child revision: D18930: Implement an "only if the rule did not match last time" policy for Herald rules.Jan 25 2018, 5:52 PM

amckinley accepted this revision.Jan 25 2018, 7:12 PM

amckinley added inline comments.

resources/sql/autopatches/20180124.herald.01.repetition.sql
2–5	Shouldn't SQL comments be either `/* */`-style or start with a leading `--`? Looks like the syntax highlighter is confused as well.

This revision is now accepted and ready to land.Jan 25 2018, 7:12 PM

I think the highlighter just implements an incomplete ruleset, since the MySQL documentation says this is OK and even lists it first:

Screen Shot 2018-01-25 at 11.17.18 AM.png (157×390 px, 19 KB)

Admittedly, these aren't super common. One soft argument for # is maybe that /* ... */ has tons and tons of spooky behavior where it's used to hide parts of statements from different versions of MySQL, and I think the degenerate form /*! .. */ is a special comment which means "this is not a comment".

I think we use /* more often though so I'll swap 'em.