Fixes #7679: Change anonymous user image url/picture_type to a null value #7715

CuriousLearner · 2018-03-04T17:37:55Z

Requesting user from API: (https://addons-server.readthedocs.io/en/latest/topics/api/accounts.html#get--api-v3-accounts-account-(int-user_id|string-username)-) will now return null in picture_type and picture_url if the profile picture is not set.

The only last bit that should be included in this patch is the migration (since models are changed). I'm having this build running with docker-compose and when I try to do make shell and then run db migrations via python manage.py makemigrations, it tells me that no changes detected.

Further to this, I explored the codebase and found that there is just one migration folder referring to all the SQL files. I'm curious to know how do we generate these files/migrations for that say.

This PR relates to an existing open issue and there are no existing
PRs open for the same issue.
Add Fixes #ISSUENUM at the top of your PR.
Add a description of the the changes introduced in this PR.
The change has been successfully run locally.
Add tests to cover the changes added in this PR.

eviljeff

the migrations in migrations are manually created, alas. You will need a snippet of sql that would change the column default. I would read the other migrations for inspiration but shout up if you can't figure it out.

eviljeff · 2018-03-05T16:55:34Z

src/olympia/users/models.py

@@ -274,7 +275,7 @@ def picture_path_original(self):
    def picture_url(self):
        from olympia.amo.templatetags.jinja_helpers import user_media_url
        if not self.picture_type:
-            return settings.STATIC_URL + '/img/zamboni/anon_user.png'


you can't change this in the Addon model because it's still used in legacy frontend views. You'll need to handle in BaseUserSerializer instead

CuriousLearner · 2018-03-07T19:08:28Z

@eviljeff I've made the changes, please have a look now.

On the issue @diox said that we use ./manage.py sqldiff to put that in migration and run that with schematic. However, when I ran the ./manage.py sqldiff users it gave me other changes too that are not the part of changes that I did.

Here is the sql that it generated:

BEGIN;
-- Application: users
-- Model: UserProfile
ALTER TABLE `users`
	MODIFY `deleted` bool;
ALTER TABLE `users`
	MODIFY `display_collections` bool;
ALTER TABLE `users`
	MODIFY `display_collections_fav` bool;
ALTER TABLE `users`
	MODIFY `notifycompat` bool;
ALTER TABLE `users`
	MODIFY `notifyevents` bool;
-- Model: UserNotification
ALTER TABLE `users_notifications`
	MODIFY `enabled` bool;
COMMIT;

Not sure if these were missed earlier or what's the case here. Please let me know :)

…alue

eviljeff · 2018-03-09T09:22:37Z

src/olympia/accounts/serializers.py

@@ -45,7 +45,10 @@ def get_permissions(self, obj):

    # Used in subclasses.
    def get_picture_url(self, obj):
-        return absolutify(obj.picture_url)
+        if obj.picture_url and \


I know it's valid python, but we don't use \ to split lines in addon-server code. Please rewrite with ( ... ) instead.

eviljeff · 2018-03-09T09:25:33Z

src/olympia/accounts/serializers.py

@@ -45,7 +45,10 @@ def get_permissions(self, obj):

    # Used in subclasses.
    def get_picture_url(self, obj):
-        return absolutify(obj.picture_url)
+        if obj.picture_url and \
+                not obj.picture_url.endswith('/img/zamboni/anon_user.png'):


Rather than testing obj.picture_url here, test obj.picture_type - because we know it'll be None now.

eviljeff · 2018-03-09T15:22:05Z

src/olympia/migrations/1001-drop-users-picture-type-not-null.sql

@@ -0,0 +1,2 @@
+ALTER TABLE `users`
+MODIFY `picture_type` varchar(75) NULL DEFAULT NULL;


This looks good to me - did you test locally with make update_db?

No idea about the auto generated SQL - you can ignore it.

I ran this using schematic src/olympia/migrations/ after I was using make shell to enable a tty on the container.

Also, I don't see if there is make udpate_db command. There is update_docker though

make update_db is an inner make command, inside the docker container - you can run it with docker-compose exec web make update_db - though it just does schematic src/olympia/migrations/ anyway so you did the same thing.

Oh, awesome! I'll keep a note of that for the subsequent PRs. Thanks :)

CuriousLearner · 2018-03-09T20:27:32Z

@eviljeff Done with the changes, can you please take another pass here?

eviljeff

Looks great - although it's not many lines of code anything involving database migrations and model changes is more complicated. r+

CuriousLearner · 2018-03-10T13:20:40Z

Thanks for the r+ @eviljeff 💃

eviljeff · 2018-03-10T13:24:30Z

We'll have to see when it deploys to the servers (-dev within the hour; stage on Tuesday, production on Thursday) if there any weird differences in the database schemas that stop the migration from running - it happens often 🤞

CuriousLearner · 2018-03-10T13:26:26Z

Oh! But why would that happen? Even if django migrations are not used, and raw SQL is ran, the SQL code would be same on dev/stage/production; isn't it?

eviljeff · 2018-03-12T08:27:59Z

When most of the tables were created there weren't many standards about what django models should create in the database (and some data even pre-dates AMO's rewrite using django 9 years ago). When you create a database locally you get the schema django wants now, which is in some cases subtlety different.

eviljeff requested changes Mar 5, 2018

View reviewed changes

Fixes #7679: Change anonymous user image url/picture_type to a null v…

f656d5c

…alue

CuriousLearner force-pushed the fix-7679 branch from 63690db to f656d5c Compare March 8, 2018 15:34

eviljeff requested changes Mar 9, 2018

View reviewed changes

Address review comments

1008d95

eviljeff approved these changes Mar 10, 2018

View reviewed changes

eviljeff merged commit 35386e9 into mozilla:master Mar 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #7679: Change anonymous user image url/picture_type to a null value #7715

Fixes #7679: Change anonymous user image url/picture_type to a null value #7715

CuriousLearner commented Mar 4, 2018 •

edited by eviljeff

Loading

eviljeff left a comment

eviljeff Mar 5, 2018

CuriousLearner commented Mar 7, 2018

eviljeff Mar 9, 2018

eviljeff Mar 9, 2018

eviljeff Mar 9, 2018

CuriousLearner Mar 9, 2018

CuriousLearner Mar 9, 2018

eviljeff Mar 10, 2018

CuriousLearner Mar 10, 2018

CuriousLearner commented Mar 9, 2018

eviljeff left a comment

CuriousLearner commented Mar 10, 2018

eviljeff commented Mar 10, 2018

CuriousLearner commented Mar 10, 2018

eviljeff commented Mar 12, 2018

		@@ -0,0 +1,2 @@
		ALTER TABLE `users`
		MODIFY `picture_type` varchar(75) NULL DEFAULT NULL;

Fixes #7679: Change anonymous user image url/picture_type to a null value #7715

Fixes #7679: Change anonymous user image url/picture_type to a null value #7715

Conversation

CuriousLearner commented Mar 4, 2018 • edited by eviljeff Loading

eviljeff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousLearner commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousLearner commented Mar 9, 2018

eviljeff left a comment

Choose a reason for hiding this comment

CuriousLearner commented Mar 10, 2018

eviljeff commented Mar 10, 2018

CuriousLearner commented Mar 10, 2018

eviljeff commented Mar 12, 2018

CuriousLearner commented Mar 4, 2018 •

edited by eviljeff

Loading