Create user profile model

[naglepuff]

No description provided.

[brianhelba]

I think you should also add an Admin page, so UserProfile can easily be changed by hand.

Also, add a column to the UserAdmin.list_display to show whether each user was verified. When you do this, ensure that the related lookup is happening via a join, not via n+1 queries.

[brianhelba]

Do you want to think about any sort of notification mechanism when new users register? We could pretty easily send an email to admins.

[brianhelba]

I think you should also add an Admin page, so UserProfile can easily be changed by hand.

Also, add a column to the UserAdmin.list_display to show whether each user was verified. When you do this, ensure that the related lookup is happening via a join, not via n+1 queries.

Actually, it makes more sense to make UserProfile be an InlineModelAdmin on User, rather than have its own top-level Admin page.

Additionally, you should add a list_filter to the UserAdmin for the verified value (allowing us to easily see only verified or only unverified users).

[naglepuff]

Do you want to think about any sort of notification mechanism when new users register? We could pretty easily send an email to admins.

Yes, I think that would be pretty helpful

[brianhelba]

Doing profile__verified doesn't work?

list_display supports this:

The name of a related field, using the __ notation. For example:

[naglepuff]

It worked but I couldn't figure out how to display the checkmark/X icons. The value in the column would just be the string "True"

[brianhelba]

This seems like a bug with Django. Can you file a bug report: https://code.djangoproject.com/query

Obviously, it's fine to keep the extra code for now then. Could you add a code comment with a link to the bug?

[naglepuff]

c438ed9

[brianhelba]

You can just put this line and the associated create_user_profiles function into 0030. No need to make two separate migration files for this.

[naglepuff]

From the docs:

Migrations that alter data are usually called “data migrations”; they’re best written as separate migrations, sitting alongside your schema migrations.

I can move it into 0030 if that's the team preference though.

[brianhelba]

I don't think it maters that much:

• Benefits of separate files: if people see auto-generated code, they might casually assume that the entire file is autogenerated; a separate file is more obvious there's a RunPython operation there
• Benefits of the same file: these migrations should be treated as atomic and ought to be run together (in the proper order) or not at all; the same file will programmatically enforce that and probably make it more obvious to anyone who's looking deeply at this file

It's totally your call.

[naglepuff]

Shouldn't they be treated as atomic and run together by nature of them being sequentially numbered migrations in the same app?

I do like putting both steps in the same migration file, and I'm happy to make the change, I'm just curious as to whether or not two instances of the same django app would intentionally migrate their database to different parts on the graph.

[naglepuff]

0640cfd

[brianhelba]

Consider the scenario that create_user_profiles has a bug, causing it to raise an Exception when it runs.

In that case (with separate migration files), Django will have successfully run 0030 to create the new table, but it will fail to have applied 0031 to create the guaranteed UserProfile for existing users. Established instances will be left in an awkward state with the table but no values. Of course, they'll also be running the new code (which assumes UserProfile), but more cautious rollouts might deploy the migration operations fully, before writing new code that depends on it.

By putting create_user_profiles as part of 0030, if the create_user_profiles function fails, the whole 0030 migration will be rolled back. The table-creation operation will be run in reverse (which is part of the reason it's good to define reverse operations), leaving instances at 0029 with no table created. Once the code bug is fixed, a new deployment will retry 0030 from scratch (since it was never marked as successful).