Page MenuHomePhabricator

Migrate old commit saved queries to new audit status constants
ClosedPublic

Authored by epriestley on Sep 10 2018, 7:50 PM.
Tags
None
Referenced Files
F12848131: D19652.diff
Fri, Mar 29, 3:12 AM
Unknown Object (File)
Sun, Mar 24, 8:23 PM
Unknown Object (File)
Wed, Mar 20, 6:41 AM
Unknown Object (File)
Wed, Mar 20, 6:41 AM
Unknown Object (File)
Feb 7 2024, 7:08 AM
Unknown Object (File)
Jan 12 2024, 1:14 AM
Unknown Object (File)
Dec 25 2023, 6:36 PM
Unknown Object (File)
Dec 14 2023, 2:41 PM
Subscribers
Restricted Owners Package

Details

Summary

Depends on D19651. Ref T13197. The application now accepts either numeric or string values. However, for consistency and to reduce surprise in the future, migrate existing saved queries to use string values.

Test Plan

Saved some queries on master with numeric constants, ran the migration, saw string constants in the database and equivalent behavior in the UI.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a subscriber: Restricted Owners Package.Sep 10 2018, 7:50 PM
amckinley added inline comments.
resources/sql/autopatches/20180910.audit.01.searches.php
7–14

I sanity-checked this against the code and it's correct, but isn't there an easy way to pull this array out of PhabricatorAuditCommitStatusConstants?

29–32

So "statuses": 1 as a constraint doesn't work? Or just malformed because diffusion.commit.search and audit.query are both expecting statuses to be list<string>, and this is just very defensive programming?

This revision is now accepted and ready to land.Sep 10 2018, 9:26 PM
epriestley added inline comments.
resources/sql/autopatches/20180910.audit.01.searches.php
7–14

I'm hard-coding the list in this migration (and two later migrations) so that I can remove the constants from PhabricatorAuditCommitStatusConstants without retaining any confusion/ambiguity going forward.

29–32

Yeah, this is mostly just defensive -- "statuses": 1 should be impossible. It also probably doesn't work.

This revision was automatically updated to reflect the committed changes.