Skip to content

Add phone_number field to UserProfile model - #23132

Merged
Tj-Tracy merged 1 commit into
masterfrom
ttracy/MICROBA-184-phone-number-field-to-profile
Feb 20, 2020
Merged

Add phone_number field to UserProfile model#23132
Tj-Tracy merged 1 commit into
masterfrom
ttracy/MICROBA-184-phone-number-field-to-profile

Conversation

@Tj-Tracy

Copy link
Copy Markdown
Contributor

MICROBA-184

Adds a simple nullable field to the UserProfile model. The only validation done it to make sure any character saved in that field is a digit and not a letter. We do not distiguish on a model level if the phone number is international or not.

@Tj-Tracy
Tj-Tracy requested review from a team February 18, 2020 19:16
@cpennington

Copy link
Copy Markdown
Contributor

Does this fall into the category of changes that AWS Aurora can do w/o locking up the table?

@Tj-Tracy

Tj-Tracy commented Feb 18, 2020

Copy link
Copy Markdown
Contributor Author

@cpennington According to the Everything About Database Migrations doc, it shouldn't cause downtime:
When using AWS Aurora, a nullable column can be added to existing large (100k rows+?) tables without causing downtime. However, the migration may still timeout in GoCD - so please coordinate the release with DevOps.

I've also let devops know this is coming.

@crice100 crice100 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we looked at the performance implications of adding a column to this table?

Never mind. I failed to read the above comments (sorry).

allow_certificate = models.BooleanField(default=1)
bio = models.CharField(blank=True, null=True, max_length=3000, db_index=False)
profile_image_uploaded_at = models.DateTimeField(null=True, blank=True)
phone_regex = RegexValidator(regex=r'^\+?1?\d*$', message="Phone number can only contain numbers.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth writing a unit test for this regex?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we only allowing US based phone numbers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to allow for international numbers on the model, and do any additional filtering elsewhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crice100 unit test added!

bio = models.CharField(blank=True, null=True, max_length=3000, db_index=False)
profile_image_uploaded_at = models.DateTimeField(null=True, blank=True)
phone_regex = RegexValidator(regex=r'^\+?1?\d*$', message="Phone number can only contain numbers.")
phone_number = models.CharField(validators=[phone_regex], blank=True, null=True, max_length=50)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember when we were going over OEP-38 in one of the Data Guild meetings, it said models should be annotated for PII. It looks like this does have the pii annotation, but do we have to add a new pii_type for "phone number"? OEP-38 points to OEP-30 here: https://github.com/edx/open-edx-proposals/blob/master/oeps/oep-0030-arch-pii-markup-and-auditing.rst.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Good catch.

Adds a simple nullable field to the UserProfile model. The only
validation done it to make sure any character saved in that field is a
digit and not a letter. We do not distiguish on a model level if the
phone number is international or not.
@Tj-Tracy
Tj-Tracy force-pushed the ttracy/MICROBA-184-phone-number-field-to-profile branch from 1f05c85 to d9af506 Compare February 20, 2020 14:44
@Tj-Tracy
Tj-Tracy requested a review from feanil February 20, 2020 14:45
@Tj-Tracy

Copy link
Copy Markdown
Contributor Author

jenkins run a11y

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@Tj-Tracy
Tj-Tracy merged commit 80320ab into master Feb 20, 2020
@Tj-Tracy
Tj-Tracy deleted the ttracy/MICROBA-184-phone-number-field-to-profile branch February 20, 2020 15:52
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants