Page MenuHomePhabricator

Add silverpop privacy delete routine
Closed, ResolvedPublic

Description

Although I'm having good success in speeding up silverpop to be incremental one gap is the privacy deletes. The only way to remove since-deleted rows from the silverpop_export table is to compare it against the civicrm_email table in it's entirety (not constrained by
modified_date).

This is not super-slow (1-2 minutes) but I think it would be locking enough to limit when we can run it.

Since I seem to be working off email_ids I think we could update the privacy delete to store a table of deleted email ids for this purpose

Event Timeline

If someone wants to pick this up I think the goal would be to add to the extension sites/default/org.wikimedia.forgetme to

  1. create a table to store the ids of forgotten emails and
  2. I think the ids could be stored in function civicrm_api3_omnirecipient_forgetme
mepps claimed this task.Jul 13 2020, 4:09 PM
mepps moved this task from Backlog to Doing on the Fundraising Sprint Nyan cats for everyone board.
mepps added a comment.Jul 13 2020, 7:23 PM

From this, I assume we'd store the email_id.

mepps added a comment.Jul 14 2020, 1:12 PM

Is there any reason we couldn't add a column to civicrm_email of is_deleted?

mepps added a comment.Jul 14 2020, 4:04 PM

So it looks like currently the row gets deleted in civicrm_email which makes sense, but we use log_civicrm_email to compare. Could we add the is_deleted row there? I'm happy to go ahead with the table of ids, but it seems so small for a unique table that I wanted to confirm it's the best option.

@mepps we delete them from the log table too!

mepps added a comment.Jul 14 2020, 7:37 PM

@Eileenmcnaughton and I assume that we send the email_id to silverpop?

mepps added a comment.Jul 14 2020, 8:51 PM

I've started on this. I need to:

  • confirm the format the emails returned, but I'm assuming it's a multidimentational array of arrays with a column key of id
  • create the new table
  • write the test--currently none of the test data uses the emails array or seems to have an email id. I'm loving the buffy test data though!
mepps added a comment.Jul 15 2020, 2:17 PM

I've added sql to add the table but I don't know how to make it part of the upgrader..

@mepps in CRM_Omnimail_Upgrader you need a function similar to this one - you can see in civicrm_extension table it has schema_version - if schema_version is less that the number at the end then when the Extension.upgrade (or update I can never remember) api is called it will run

/**
 * @throws \CiviCRM_API3_Exception
 */
public function upgrade_1003() {
  $this->addCustomFields();
  return TRUE;
}
mepps added a comment.Wed, Jul 15, 8:36 PM

Thanks @Eileenmcnaughton! That worked.

Change 612949 had a related patch set uploaded (by Mepps; owner: Mepps):
[wikimedia/fundraising/crm@master] Upgrade forget to store deleted email ids

https://gerrit.wikimedia.org/r/612949

Okay I've got my patch working, but still need a test. I'll move this into review but I'd like to add a test before it's +2ed.

I think you could probably just add a check to one of the existing tests. It all looks good - I just can't quite spot the install issue

mepps added a comment.Thu, Jul 16, 5:28 PM

Yay! It's tested.

mepps added a comment.Mon, Jul 20, 1:26 PM

@Eileenmcnaughton what's the install issue?

Change 612949 merged by Eileen:
[wikimedia/fundraising/crm@master] Upgrade org.wikimedia.forgetme to store deleted email ids

https://gerrit.wikimedia.org/r/612949

@mepps I just deployed this - my intention was then to use that table in the silverpop script to delete those ids from silverpop staging in place of this query

  • Remove any privacy deletes by getting rid of emails no longer in the email table.
  • This is a relatively long time to lock this table. It takes 1-2 minutes longer
  • if we use the log_civicrm_email table and I'm not sure it's any less bad to lock
  • that instead. A better way would be to temporarily record the emails to delete.
  • Query OK, 243 rows affected (1 min 0.45 sec)
  • https://phabricator.wikimedia.org/T257001

DELETE s FROM silverpop_export_staging s

LEFT JOIN civicrm.civicrm_email l
ON s.id = l.id

WHERE l.id IS NULL;

and also to check out the other tables.

I was going to suggest you might pick up that step- but I'm not quite sure which ticket it would be against. I also talked with Jack about him maybe picking up the unsubscribe ticket in the next sprint - because I think now I've made the changes I thought needed doing it's a good time to move me from the driver's seat to the back-seat driver's seat on this code & get some more heads into it to spot the things I might not have thought about

DStrine closed this task as Resolved.Tue, Jul 21, 7:03 PM