for MPP-2223: add RelayNumber.enabled field

[groovecoder]

New feature description

Adds a PATCH /api/v1/relaynumber/{id} endpoint to enable/disable the new RelayNumber.enabled field, which controls forwarding/blocking all calls and messages.

Screenshot (if applicable)

How to test

1. Use the new endpoint at http://127.0.0.1:8000/api/v1/docs/ to set a relay number to enabled: true.
2. Send a text to the relay number
   • It should be forwarded
3. Use the new endpoint at http://127.0.0.1:8000/api/v1/docs/ to set a relay number to enabled: false.
4. Send a text to the relay number
   • It should NOT be forwarded

Checklist

• All acceptance criteria are met.
• Commits in this PR are minimal and have descriptive commit messages.

[netlify]

✅ Deploy Preview for fx-relay-demo canceled.

Name	Link
🔨 Latest commit	7cd0dfd
🔍 Latest deploy log	https://app.netlify.com/sites/fx-relay-demo/deploys/62e45e39ce7d930008044b5e

[groovecoder]

This change looks weird. Basically, I saw that these 2 tests (test_relaynumber_suggestions_bad_request_for_user_without_real_phone and test_relaynumber_post_with_existing_returns_error) had the wrong names. So I switched the names so the test names would match what the test is actually doing. But this is how git interpreted that change. 🤷

[jwhitlock]

All new fields either need nullable=True, or we need to set a server default in the migration. Otherwise, we'll have problems with the deploy, when the existing servers that don't know about the new field try to write to the database migrated by the new code.

The nullable=True has been a real pain, let's try to figure out the server default...

[groovecoder]

Hrm ... when I ran the migration on my local it didn't complain. Maybe I didn't have any existing records when I ran it? Or does django know enough that if default=True is in the field definition, it sets all existing records to True ?

[jwhitlock]

Update: Added the PostgreSQL version, fixed the sqlmigrate command*

The problem is not running the migration, the problem is when we deploy it. The Kubernetes rollout preserves the number of web pods by replacing old ones with new ones, one at a time, so the old pods will run against the new database schema. You can replicate this with:

1. Check out this PR
2. Run migration 0017
3. Check out tag v3.6.1 (or main, close enough)
4. Go through the steps to create a Relay number (register a real phone, setup a relay number)

Steps 4 is hard, but you'll end up where the old code is running:

INSERT INTO "phones_relaynumber"
   ("id", "number", "location", "user_id", "vcard_lookup_key")
   VALUES (123, '+1234567890', 'location', 456, 'acb123');

resulting in the error:

django.db.utils.IntegrityError: phones_relaynumber.enabled NOT NULL constraint failed

It was a little easier to see with PR #2232, where a integer column was added, and you could use the UI to fail to create a new alias.

So, we'll get database errors and data loss during the rollout. There's two ways to deal with this. One is to add null=True, the other is a database default.

The deploy sequence with null=True:

1. Deploy One:
   • A migration adds a column with NULL allowed.
   • Old code omits the new column on inserts and updates, and new rows get a null value.
   • The new code needs to expect a null value and interpret it as the default (in this case True).
2. Deploy Two:
   • A data migration replaces all NULL values with True, and a schema migration changes NULL to NOT NULL.
   • The old code continues to set to true or false, and checks for null even though is won't happen.
   • The new code can drop the null checks and assume only true or false.

The deploy sequence with a database default:

1. Deploy One:
   • A migration adds a column with a database default
   • Old code omits the new column on inserts and updates, and new rows get the database default
   • The new code gets either true or false, never null
2. Deploy Two:
   • A migration drops the database default
   • The old code and the new code are identical, always setting the column to true or false

The database default is a lot cleaner - the code doesn't change, and no mucking about with null values. The problem is that Django doesn't support database defaults (issue #470), so you can't do it without writing your own migrations. And that is hard because we have to support both SQLite3 and PostgreSQL.

You can see what Django does with ./manage.py sqlmigrate phones 0017:

BEGIN;
--
-- Add field enabled to relaynumber
--
CREATE TABLE "new__phones_relaynumber" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "enabled" bool NOT NULL, "number" varchar(15) NOT NULL, "location" varchar(255) NOT NULL, "user_id" integer NOT NULL REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED, "vcard_lookup_key" varchar(6) NOT NULL UNIQUE);
INSERT INTO "new__phones_relaynumber" ("id", "number", "location", "user_id", "vcard_lookup_key", "enabled") SELECT "id", "number", "location", "user_id", "vcard_lookup_key", 1 FROM "phones_relaynumber";
DROP TABLE "phones_relaynumber";
ALTER TABLE "new__phones_relaynumber" RENAME TO "phones_relaynumber";
CREATE INDEX "phones_relaynumber_number_742e5d6b" ON "phones_relaynumber" ("number");
CREATE INDEX "phones_relaynumber_user_id_62c65ede" ON "phones_relaynumber" ("user_id");
COMMIT;

The sequence is:

• Create a new table new__phones_relaynumber with the enable column
• Copy all the data from phones_relaynumber, setting the enable column to 1 (true)
• Drop phones_relaynumber
• Rename new__phones_relaynumber to phones_relaynumber
• Re-add the indexes

This is what the Django docs say for sqlite3 migrations:

SQLite has very little built-in schema alteration support, and so Django attempts to emulate it by:

• Creating a new table with the new schema
• Copying the data across
• Dropping the old table
• Renaming the new table to match the original name

The PostgreSQL looks like:

BEGIN;
--
-- Add field enabled to relaynumber
--
ALTER TABLE "phones_relaynumber" ADD COLUMN "enabled" boolean DEFAULT true NOT NULL;
ALTER TABLE "phones_relaynumber" ALTER COLUMN "enabled" DROP DEFAULT;
COMMIT;

There's a note for PostgreSQL:

The only caveat is that prior to PostgreSQL 11, adding columns with default values causes a full rewrite of the table, for a time proportional to its size. For this reason, it’s recommended you always create new columns with null=True, as this way they will be added immediately.

We're running PostgreSQL 11.15.

[jwhitlock]

Thanks @groovecoder, I'm glad we're adding enabling / disabling this early in the feature!

Please run black on the codebase, and consider merging PR #2261 into this PR.

[groovecoder]

@jwhitlock this is ready for a final(?) review.

[jwhitlock]

Thanks @groovecoder! I checked it out, it looks good.

I think that instead of calling client.messages.create in api.views.phones.inbound_sms, we could respond with a Message verb:

<?xml version="1.0" encoding="UTF-8"?>
<Response>
    <Message to="+user_real_num" from="+mask_num">
        [Relay 📲 +incoming_num] Message
    </Message>
</Response>

but that's a different experiment.

[groovecoder]

I filed https://mozilla-hub.atlassian.net/browse/MPP-2232 for us to look into returning TwiML to forward messages.