Page MenuHomePhabricator

Support designating a contact number as "primary"
ClosedPublic

Authored by epriestley on Jan 21 2019, 10:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 18, 9:55 PM
Unknown Object (File)
Fri, Mar 15, 11:51 PM
Unknown Object (File)
Sun, Mar 10, 3:42 PM
Unknown Object (File)
Wed, Mar 6, 10:40 PM
Unknown Object (File)
Thu, Feb 29, 4:51 PM
Unknown Object (File)
Feb 9 2024, 1:32 PM
Unknown Object (File)
Feb 3 2024, 8:53 PM
Unknown Object (File)
Jan 31 2024, 6:02 AM
Subscribers
Restricted Owners Package

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
Branch
mfa8
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21636
Build 29502: Run Core Tests
Build 29501: arc lint + arc unit

Event Timeline

Owners added a subscriber: Restricted Owners Package.Jan 21 2019, 10:32 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.Jan 21 2019, 11:47 PM
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.