[Bug 1418276] [NEW] Deleting a user via the REST API does not delete their user preferences
Public bug reported: The on_delete function in rest/users.py appears to not delete user preferences when the user is deleted resulting in an accumulation of orphaned preferences data. def on_delete(self, request, response): """Delete the named user, all her memberships, and addresses.""" if self._user is None: not_found(response) return for member in self._user.memberships.members: member.unsubscribe() user_manager = getUtility(IUserManager) for address in self._user.addresses: user_manager.delete_address(address) user_manager.delete_user(self._user) no_content(response) ** Affects: mailman Importance: Undecided Status: New -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
** Tags added: mailman3 -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
** Changed in: mailman Assignee: (unassigned) => Ankush Sharma (black-perl) -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
** Changed in: mailman Assignee: Ankush Sharma (black-perl) => Abhishek (abhi170893) -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
Deleted the preferences of the user being deleted before deleting him. ** Patch added: "delete_preferences_on_user_deletion" https://bugs.launchpad.net/mailman/+bug/1418276/+attachment/4349590/+files/d... ** Changed in: mailman Status: New => In Progress -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
Deleted the preferences of the user being deleted before deleting him and added test for it. ** Attachment added: "contains solution and test for it." https://bugs.launchpad.net/mailman/+bug/1418276/+attachment/4350705/+files/p... -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
Thanks for the patch and the test, it's a great contribution. I am going to apply it with some modifications. Here is some feedback for the future: Please read PEP 8 and the Mailman style guide for our coding standards. Because the fix is in the model, there should be a test in the model. It's okay to also have a test in the REST layer because that's where the problem was observed. I adapted your REST test and added one to the model. Be sure to run the full test suite with `tox` both before and after applying your fix. It's a good idea to add the test, run tox to validate that the test fails, then add your fix to validate that it succeeds. Also, running the full suite before submitting a patch ensures that there are no regressions elsewhere in Mailman. When attributing fixes by community members, I use the name given in your Launchpad id. If you want your full name to appear in the NEWS file, please contact me directly. See r7307 for the full, applied patch. ** Changed in: mailman Milestone: None => 3.0.0b6 ** Changed in: mailman Importance: Undecided => High ** Changed in: mailman Status: In Progress => Fix Committed -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
Thanks Barry for the suggestions. I will see the PEP 8 and the Mailman guide. And i tried pushing the branch for the merge proposal, but from inside the college i cant do ssh outside and was getting error. So i submitted the patch. I looked at the r7307 and wanted to ask something: In rest/tests/test_users.py you have used config.db.store.query(Preferences) inside with. But there is another usage of it in line 221 and also in the file model/tests/test_user.py which are not inside with block. Any specific reason for the difference..? 230 with transaction(): 231 preferences = config.db.store.query(Preferences).filter_by( 232 id=anne.preferences.id) 233 self.assertEqual(preferences.count(), 0) -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
On Mar 20, 2015, at 05:41 PM, Abhishek wrote:
In rest/tests/test_users.py you have used config.db.store.query(Preferences) inside with. But there is another usage of it in line 221 and also in the file model/tests/test_user.py which are not inside with block. Any specific reason for the difference..?
230 with transaction(): 231 preferences = config.db.store.query(Preferences).filter_by( 232 id=anne.preferences.id) 233 self.assertEqual(preferences.count(), 0)
Yes. The ids don't get assigned until after the commit, so they have to be within the with-statement in order for the subsequent query to work. But after that, the delete does not require a commit for the query to succeed. I don't think this was the case with Storm, but it appears to be so with SQLAlchemy.
** Changed in: mailman Status: Fix Committed => Fix Released -- You received this bug notification because you are a member of Mailman Coders, which is subscribed to GNU Mailman. https://bugs.launchpad.net/bugs/1418276 Title: Deleting a user via the REST API does not delete their user preferences To manage notifications about this bug go to: https://bugs.launchpad.net/mailman/+bug/1418276/+subscriptions
participants (5)
-
Abhishek -
Andrew Stuart -
Ankush Sharma -
Barry Warsaw -
Barry Warsaw