Page MenuHomePhabricator

Support designating a contact number as "primary"
ClosedPublic

Authored by epriestley on Mon, Jan 21, 10:32 PM.

Details

Summary

Depends on D20010. Ref T920. Allow users to designate which contact number is "primary": the number we'll actually send stuff to.

Since this interacts in weird ways with "disable", just do a "when any number is touched, put all of the user's rows into the right state" sort of thing.

Test Plan
  • Added numbers, made numbers primary, disabled a primary number, un-disabled a number with no primaries. Got sensible behavior in all cases.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mon, Jan 21, 10:32 PM
Owners added a subscriber: Restricted Owners Package.Mon, Jan 21, 10:32 PM
epriestley requested review of this revision.Mon, Jan 21, 10:34 PM
amckinley accepted this revision.Mon, Jan 21, 11:47 PM

Do we have other examples of this kind of "exactly one row must have columnFoo set to bar" logic elsewhere? Off the top of my head I can think of primary email addresses. Wondering if it's worth factoring out into the data layer instead of mangling it like we do here.

src/applications/auth/controller/contact/PhabricatorAuthContactNumberPrimaryController.php
36–43

Maybe just don't show the button?

73

"Designate"

This revision is now accepted and ready to land.Mon, Jan 21, 11:47 PM
epriestley added inline comments.Tue, Jan 22, 12:06 AM
src/applications/auth/controller/contact/PhabricatorAuthContactNumberPrimaryController.php
36–43

(I think there's significant value in having the UI remain consistent, even though remotely reasonable users probably won't end up here.)

  • Use a conventional spelling of "designate".

Do we have other examples of this kind of "exactly one row must have columnFoo set to bar" logic elsewhere?

Sort of. Email addresses have a similar "primary" flag, although it's simpler because they can't currently be disabled. The logic there is a little more direct (in PhabriactorUserEditor->changePrimaryEmail()): load the old primary mail, un-make it primary, save it, make the new one primary, save it.

I think that version of things isn't too bad, although it doesn't converge to the right state under an arbitrary user running UpDaTe ... queries and marking multiple rows as isPrimary. It also relies on a heavier transactional lock with a longer lock window, although this is probably not a real concern.

Neither version prevents the database from being put in an invalid state with UpDaTe .... In the email case (where there's no "disabled" state), we can make that guarantee like this:

  • Make isPrimary nullable.
  • Store NULL to mean "not primary".
  • Pick a particular value, probably "1", to mean "primary".
  • Add a UNIQUE KEY (userPHID, isPrimary).

However, I generally think this approach is too messy to really embrace. In particular:

  • To understand it, you have to know that when a row is bound by a unique key but has a NULL value for one of the columns named in the key, it is exempted from uniqueness constraints. I generally assume that the "typical" engineer knows a bit about keys but may not understand multipart keys in much detail, much less be comfortable with UNIQUE + NULL tricks.
  • It means that isPrimary = 0 and isPrimary = NULL are two different possible states that should have the same meaning (and only one row is allowed to have isPrimary = 0).
    • Code like setIsPrimary(0) will work if you test it, then fail if you go through the same flow again. Surprise!
    • The correct code becomes setIsPrimary(NULL) or there needs to be a translation layer.
  • We're out of luck anyway if we add a rule like "and disabled things can't be primary", as here, and we have to accept that ambitious users may UpDaTe ... us into an invalid state.

In this case, I think that the better long-term way to represent this might be something like a different table with <userPHID, contactNumberRole, contactNumberPHID> so you can have a "Primary MFA Number" and "Primary Pager Number" as separate entries (and they can be the same number or different numbers, and third party applications can define new roles without schema changes...) but I don't want to build out too much of that stuff so far ahead of the game.

In general this pattern is fairly rare. I'm kind of experimenting with this approach here, but all the ways I know of to handle this feel like they have roughly-similar tradeoffs in different directions.

This revision was automatically updated to reflect the committed changes.