This is a minor point, but looking through the code it appears that the pm_email_notify table only ever gets updated or inserted. When users are deleted there is no code to delete their corresponding entry in the pm_email_notify table. One solution might be to add a hook user (pm_email_notify_user) function to perform cleanup on $op = 'delete'.

CommentFileSizeAuthor
#16 privatemsg_cancel_user2.patch6.6 KBBerdir
#14 privatemsg_cancel_user.patch5.8 KBBerdir
#12 user_delete_stuff2.patch6.33 KBBerdir
#11 user_delete_stuff.patch4.7 KBBerdir
#2 pm_email_notify.module.patch433 bytessja1
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

litwol’s picture

litwol CreditAttribution: litwol commented
Version: 6.x-1.0-rc3 » 6.x-1.x-dev
Category: bug » task

That's not a bug. its a feature that was not considered oritinally. Good idea. Your approach is good (hook_user $op='delete'). Uplod a patch of work in progress so others can help out.

sja1’s picture

sja1 CreditAttribution: sja1 commented
Status: Active » Needs review
FileSize
pm_email_notify.module.patch433 bytes

Here's a swipe at it...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Looks good, simple change.

litwol’s picture

litwol CreditAttribution: litwol commented
Status: Reviewed & tested by the community » Active

Patch looks good but i am setting this back to active for the sakes of the following discussion (Deppends on what we decide i will change this back to RTBC or needs work):

Similar issue to how user deletions are now handled in 7 core, we should consider having similar integration in privatemsg. What exactly do we clean up when user is deleted/disabled/etc.. Do we delete all of the user settings. Do we delete all of the user messages. Do we archive them somehow. Do we add some kind of marker that user is deleted even though he appears in the "from/to" fields. that leads into next problem of if user is deleted then we cant do join on the user table to fetch his user name to appear in the 'from/to' field.

Much to discuss, what do you guys think?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Well, Drupal 7 contains a proper facility to cancel accounts, and it also allows to configure what to do with the contents of that user so that we can properly hook into that.

Not sure what to do for Drupal 6...

litwol’s picture

litwol CreditAttribution: litwol commented

My gut feeling want me to say: Lets have a new admin UI element to allow site admin to chose behavior for user deletions 1) keep the messages/settings/etc.. 2) delete the messages/settings/etc.

However on real usecase side of things it leads me to believe that it makes absolutely no sense to keep messages/settings/etc... in database after user was deleted because we will never be able to fetch necessary information such as user name or make joints on the users table. So it makes me think that we should also perform complete cleanup when user is deleted without giving site administrator an option to cancel this behavior.

sja1’s picture

sja1 CreditAttribution: sja1 commented

I think it makes more sense to perform cleanup, and leave configurable deletion handling options for a D7 version of the module. Certainly there is no value in retaining user settings anyway for deleted users, the settings provide no useful historical information.

Thinking about this, I am now wondering about a different problem that I'll open in a different issue.

litwol’s picture

litwol CreditAttribution: litwol commented
Status: Active » Needs work

Seems we've reached a consensus. Lets make a patch that encompases all aspects of pmsg user content/settings cleanup (not just email notification settings).

sja1’s picture

sja1 CreditAttribution: sja1 commented

I'm not very familiar with the rest of the module. What else needs to get cleaned up?

litwol’s picture

litwol CreditAttribution: litwol commented

Have a quick scan through the module and it's feature-set...

I think we also need to clean up the actual messages as well. There could be more.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
user_delete_stuff.patch4.7 KB

I'm in patch writing mood today :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: PM Email Notify doesn't clean its table when users are deleted » Delete messages and settings when a user is deleted.
FileSize
user_delete_stuff2.patch6.33 KB

Updates:

- Added the test from #615834: Error if adressed to someone who got deleted since the patch changed and doesn't use them anymore.
- It does now also delete recipient entries that belong to messages written by that user.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

This has been commited to 6.x-1.x-dev.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
privatemsg_cancel_user.patch5.8 KB

Updated the patch to D7. This does now use the user_cancel stuff.

It also contains a test when the user is actually deleted.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Patch (to be ported) » Needs review
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
privatemsg_cancel_user2.patch6.6 KB

Integrated the bug fix including a test case from #626692: Deleting user causes mysql error.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Fixed

Commited against 7.x-1.x-dev.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.