Bug 1005127 - [privacy] Add ability to delete profile #141

bitgeeky · 2014-06-26T18:36:06Z

Works fine for me.
It required to add some new mixins, views and unit tests.

bitgeeky · 2014-06-26T18:36:26Z

bobsilverberg · 2014-06-26T19:11:29Z

...ks/migrations/0011_auto__chg_field_taskattempt_user__chg_field_task_creator__chg_field_ta.py

+        # Changing field 'TaskAttempt.user'
+        db.alter_column('tasks_taskattempt', 'user_id', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], null=True, on_delete=models.SET_NULL))
+
+        # Changing field 'Task.creator'


This migration does not look right to me. The only thing it should do is change TaskAttempt.user. All of the other changes are not needed and should not be included. Only administrators can add records to those other tables, and their accounts will never be deleted.

I just ran "./manage.py schemamigration oneanddone.tasks --auto" and added default value to be None whenever prompted. May be you can just remove this file when you test it and run the migration yourself. Let me know what it gives for you.

I hope you will also get the same file as this one.

Ah, I get it. When you set the database up so that when a user is deleted it sets the TaskAttempt record's user field to NULL, I guess it interpreted that as being applicable to all of the other places where user is a foreign key. I don't think we need all of those changes. We may have to edit the migration manually.

I think we're going to have to live with this. You may have to rename the migration as there is another migration already assigned the 0011 number. Please rename the migration file to start with 0012.

bobsilverberg · 2014-06-26T19:14:52Z

Why is there so much stuff in this pull request? This is only supposed to be about deleting a user profile. I think you must have other commits in here which got squashed before being pushed to master. Please clean this up and resubmit it. Perhaps the easiest way to do that would be to start with a branch based on the latest master branch, and then do a git cherry-pick of this commit and resolve any conflicts. I think that will give you a clean commit.

bitgeeky · 2014-06-26T19:26:46Z

All the "stuff" is added intentionally. There are no extra commits.
Deletion wasn't as simple as it seemed to be.

bitgeeky · 2014-06-26T19:27:54Z

Please review and let me know what you feel should not be there and I will give you the answers why I added that.

bobsilverberg · 2014-06-26T20:04:45Z

Why did you have to make changes to the mixins and the mixins that were being used by the views. Everything seemed to be working fine before this, so why are these changes necessary?

bobsilverberg · 2014-06-26T20:13:06Z

oneanddone/users/templates/users/profile/create.html

@@ -0,0 +1,37 @@
+{# This Source Code Form is subject to the terms of the Mozilla Public


This file is almost identical to edit.html. This results in too much duplicate code. Please find a way to use include to reduce the duplication.

Both of these are separate forms and have target to different views although they have some fields in common.
I looked more into this but couldn't find a way that could achieve what we want to do here.
@bobsilverberg Please let me know if you know of any solution for this, I will be happy to make the change.

Ouch !! sorry got confused with something else. Fixed this. Ignore the comment.

bobsilverberg · 2014-06-26T20:55:38Z

There is a bug in the code. If you delete your profile and click confirm, it first sends you to the home page and it looks logged out, but then it redirects you to the profile/new/ page and you are still logged in. You need to find a way to actually log them out of the site.

Osmose · 2014-06-27T03:32:06Z

There is a bug in the code. If you delete your profile and click confirm, it first sends you to the home page and it looks logged out, but then it redirects you to the profile/new/ page and you are still logged in. You need to find a way to actually log them out of the site.

This sounds like Persona attempting to log you back in because navigator.id.logout() wasn't called in the JavaScript. I checked and One and Done appears to be using django-browserid 0.9, which doesn't disable this behavior[1]. If you upgrade the django-browserid 10.1, you should be able to avoid the auto-login that might be causing this.

[1] Specifically, Persona maintains a session for users when they log into your site. If they show up at your site and they aren't logged into your site, but navigator.id.logout() wasn't called in your site's JavaScript at some point, Persona tries to be helpful and trigger and automatic login because it thinks the user wants to be logged in to your site.

It is incredibly confusing and hard to debug and they recently added a way to disable it (which I have a PR for in the django-browserid repo).

bobsilverberg · 2014-06-27T04:16:02Z

Thanks for the info, @Osmose. I tried to upgrade django-browserid in the past and was unsuccessful (errors on staging that I couldn't debug), so I backed it out. I can try again, but we have a really tight timeline so I'm not sure it's something I can do by Monday.

Can we do something to manually call navigator.id.logout() after deleting the user account? Would that temporarily solve the issue? Also, you say you have a PR for the issue in the django-browserid repo. Does that mean upgrading to the current version won't fix the problem, and that we'd need a version that includes your outstanding PR? I'm a bit confused by those statements.

bitgeeky · 2014-06-27T06:07:45Z

I made the suggested changes leaving that Persona bug. We should wait for more info from @Osmose on that and how to fix it here.

I observed that the force login by Persona occurs if you delete and logout a few times in a row. For single delete and logout it works fine.

It will be great if we can fix that, but in the worst case we can live with it (assuming user will delete his profile once only and not experiment with the things too much)

Osmose · 2014-06-27T11:04:19Z

Can we do something to manually call navigator.id.logout() after deleting the user account? Would that temporarily solve the issue?

If, post-deletion, you redirect to a specific post-deletion view, you can add the JS to that page that calls navigator.id.logout(). Since the base JavaScript bundle (included here) includes api.js and browserid.js from django-browserid, you may run into an issue where api.js calls navigator.id.watch() before you can call navigator.id.logout(). Since the auto-login happens when navigator.id.watch() is called, you'll want to set up the include order of the JS files for that particular page such that navigator.id.logout() gets called first.

Also, you say you have a PR for the issue in the django-browserid repo. Does that mean upgrading to the current version won't fix the problem, and that we'd need a version that includes your outstanding PR? I'm a bit confused by those statements.

No, django-browserid 0.10.1 already had a workaround in place. The aforementioned PR switches us from using our workaround to using the new API that disables it. 0.10 and above will not perform any auto-login.

bitgeeky · 2014-06-27T11:29:14Z

@bobsilverberg can we please try to upgrade the django-browserid version ?
We are anyways going to need it later at some point of time also there are more chances of error if we write custom code.

bobsilverberg · 2014-06-27T12:16:20Z

@bitgeeky I am not sure there will be time to do that now. It is on the roadmap for the next release, so we should not count on it being done now. You should try to implement Osmose's recommended workaround now, as we need a solution to this immediately.

Regarding your comments about when it happens: with my testing it happened every time I deleted a user. When I delete the user I am first directed to the home page, and then I am directed to the profile/new/ page, so this is something that definitely needs to be addressed. Please make implementing Osmose's suggested workaround your top priority.

bobsilverberg · 2014-06-27T14:27:58Z

oneanddone/tasks/views.py

@@ -17,7 +17,7 @@
 from oneanddone.tasks.mixins import TaskMustBePublishedMixin
 from oneanddone.tasks.models import Feedback, Task, TaskAttempt
 from oneanddone.tasks.serializers import TaskSerializer
-from oneanddone.users.mixins import MyStaffUserRequiredMixin, UserProfileRequiredMixin
+from oneanddone.users.mixins import MyStaffUserRequiredMixin, UserProfileRequiredMixin, PrivacyPolicyRequiredMixin


Now that UserProfileRequiredMixin has effectively been replaced by PrivacyPolicyRequiredMixin, you no longer need to import it.

bobsilverberg · 2014-06-27T14:45:59Z

@bitgeeky I have finished my latest review. Please address the issues, plus I know you are also working on the logout issue. Also, there are now conflicts, but we can deal with those once the PR is ready to merge.

bitgeeky · 2014-06-27T14:50:58Z

I made the changes suggested by @Osmose. Logout works pretty fine for me now. @bobsilverberg please test it and let me know if it works fine for you also.
Logout is the main priority. I will address other issues once logout works fine.

bobsilverberg · 2014-06-27T15:08:28Z

The logout code works! Great work, @bitgeeky! Let's get the rest of the changes in so we can merge this. We are very close to having all the features done for this release. :)

bitgeeky · 2014-06-27T16:37:11Z

Made the suggested changes ! @bobsilverberg r ?

bobsilverberg · 2014-06-27T17:00:33Z

Looks good, @bitgeeky. Can you resolve the conflicts and squash the commits and then we'll merge this. Good work!

bobsilverberg · 2014-06-27T17:33:26Z

This has landed in e67fe32

bobsilverberg · 2014-06-27T17:38:38Z

Note that I renamed the migration to 0010... because it will be run before the others that had that number. I'll need to rename the others.

Bug 1005127 - [privacy] Add ability to delete profile

80449cb

bobsilverberg reviewed Jun 26, 2014
View reviewed changes

Made the suggested changes

4029246

bobsilverberg reviewed Jun 27, 2014
View reviewed changes

Made changes suggested by Osmose

7699896

Very small changes

7b39164

bobsilverberg closed this Jun 27, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1005127 - [privacy] Add ability to delete profile #141

Bug 1005127 - [privacy] Add ability to delete profile #141

bitgeeky commented Jun 26, 2014

bitgeeky commented Jun 26, 2014

bobsilverberg Jun 26, 2014

bitgeeky Jun 26, 2014

bitgeeky Jun 26, 2014

bobsilverberg Jun 26, 2014

bobsilverberg Jun 26, 2014

bobsilverberg commented Jun 26, 2014

bitgeeky commented Jun 26, 2014

bitgeeky commented Jun 26, 2014

bobsilverberg commented Jun 26, 2014

bobsilverberg Jun 26, 2014

bitgeeky Jun 27, 2014

bitgeeky Jun 27, 2014

bobsilverberg commented Jun 26, 2014

Osmose commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

Osmose commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bobsilverberg Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

		@@ -0,0 +1,37 @@
		{# This Source Code Form is subject to the terms of the Mozilla Public

Bug 1005127 - [privacy] Add ability to delete profile #141

Bug 1005127 - [privacy] Add ability to delete profile #141

Conversation

bitgeeky commented Jun 26, 2014

bitgeeky commented Jun 26, 2014

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

bobsilverberg commented Jun 26, 2014

bitgeeky commented Jun 26, 2014

bitgeeky commented Jun 26, 2014

bobsilverberg commented Jun 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobsilverberg commented Jun 26, 2014

Osmose commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

Osmose commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

Choose a reason for hiding this comment

bobsilverberg commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bitgeeky commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014

bobsilverberg commented Jun 27, 2014