Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing Cascading Deletion of Events #2194

Closed
5 tasks
mnelson4 opened this issue Jan 10, 2020 · 9 comments
Closed
5 tasks

Testing Cascading Deletion of Events #2194

mnelson4 opened this issue Jan 10, 2020 · 9 comments

Comments

@mnelson4
Copy link
Contributor

mnelson4 commented Jan 10, 2020

Issue Overview

Although I've been getting some feedback during development, I haven't been putting work on hold for it too much either.
Anyways, with all the pieces in place, this needs to be officially tested.

Testing Checklist

  • Create an event with no registrations. Trash it then permanently delete it. It should be deleted fine (you may want to check the database that its datetimes, tickets, message-group relations, etc, were removed properly.)
  • Create an event with some registrations and do the same. Those registrations should also be deleted.
  • Visit the transaction pages for one of those registrations. It won't have any registrations, but it should still be a valid record of a real transaction and payment. (Deleting transactions and payments is another job for another day.)
  • Create an event that uses some custom questions, payments, and registrations and do the same. Check those registrations' answer rows are also deleted.
  • Create a few events with registrations, and bulk trash them then bulk permanently delete them. They should all get deleted fine.
  • Repeat the above but with some add-ons active that add data related to events or registrations. Like promotions (which has Promotion_Object model which relates promotions to events; or Infusionsoft which records extra meta about events that are sync'd; or MailChimp which has other tables pointing to events; or payment methods.) It may not be a hard requirement that this always delete those add-ons' data, but it certainly shouldn't have any errors, and it would be great if this PR did clean up their data.
  • Create an event with 1000 registrations and do the same. It will take a few minutes to generate the preview page, and a minute to delete, but it should work. Preferably do this test from a cheap shared host (I have one on Dreamhost if that helps). Note though the event can't be infinitely big, I know it doesn't work deleting an event with over 15,000 registrations (in which I outlined a solution on Perform cascading deletion in batches #2110 (comment))
@joshfeck
Copy link
Contributor

Has there been consideration for how the orphaned transactions will be deleted?

The current functionality when you trash, then permanently delete a registration is it will also delete the transaction too. It seems like the transaction should also be deleted too. Otherwise how will they be deleted without going in and directly running deleted commands on the database?

@mnelson4
Copy link
Contributor Author

mnelson4 commented Jan 13, 2020

Has there been consideration for how the orphaned transactions will be deleted?

Good thing to bring up. You mentioned it, but I must have glossed over the fact there's no UI to delete transactions.

Regardless, I thought that was going to be the next part of the cascading deletions epic. Deleting transactions was out-of-scope for this first project. I'm sorry if I didn't communicate that properly.

Anyways, currently transactions with no registrations don't actually appear in the transactions list table (it's because of a bug in the transactions list table query, which isn't too hard to fix.) So they eat up some disk space, but many users won't know about them, if that's any consolation at all.

I thought during the next cascading deletions project we would want to provide a way to delete transactions, and clean up transactions with no registrations. Here's some rough ideas:

  • show transactions with no registrations in the transactions list table, and add links to delete them
  • add a link to delete all transactions with no registrations from the system maintenance page
  • add an option to the event deletion page, asking the admin if they want to also delete transactions and payments.

Do you think we should do some of that in this first project first @joshfeck? If so, what would you like?

@joshfeck
Copy link
Contributor

Current behavior (in master branch) when trashing registrations is it checks for other registrations in the same transaction, and if all registrations are in the trash, it goes ahead and deletes the related transaction & line items. If there is a registration from the same transaction that's not trashed then it doesn't delete the transaction. This is in the _delete_registration() method of the Registrations_Admin_Page class.

Could the cascade delete do something similar?

@mnelson4
Copy link
Contributor Author

Could the cascade delete do something similar?

I don't have a problem with extending the project if others don't. Theoretically that will be pretty simple but it will probably be a week or so more for the project. I'm investigating.

@mnelson4
Copy link
Contributor Author

KK I created #2225 and passed to Brent for initial review (I purposefully did a quick-n-dirty change just to verify the logic; I even suggested some moderate code improvements I'd like to do afterwards.)

@joshfeck
Copy link
Contributor

joshfeck commented Jan 17, 2020

  • This string will need to be removed because Contacts aren't allowed to be deleted:
    "You can delete the contacts afterwards if you like."

  • We're probably going to need to change the method of verifying before deleting. Seth suggested a checkbox next to each event in a list instead of requiring the user to input the URL slug of every event.

  • If all events in the selection have no registrations could it skip the preview and verification step and go right to permanently deleting? That's the current behavior in master and this hasn't been a problem. -> Mike: We discussed this, and the preview page is less important when deleting events with no registrations. But this suggestion would add some more controller code (which could, like any code, have more bugs and will require more maintenance) so we decided not to do it.

@joshfeck
Copy link
Contributor

OK to continue testing on CASC/delete-transactions3 or should I be testing on another branch?

Is any merging of branched needed before final testing?

@mnelson4
Copy link
Contributor Author

Please test CASC/delete-transactions3 (I didn't merge it to CASC/base in case there's some unforseen problem with it and we decide to do it another way). If the testing notes from #2248 are OK, then we can merge that into CASC/base, and continue with this ticket's more general testing checklist.)

@mnelson4
Copy link
Contributor Author

I don't really know why I made a ticket for this when there was already a PR for the main branch CASC/base. Moving testing notes onto that PR and closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment