When you delete a status, signups are left with bogus data
dww - September 19, 2009 - 18:40
| Project: | Signup Status |
| Version: | 6.x-1.0-alpha2 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | dww |
| Status: | closed |
Description
While working with #581846: Cleanup the signup_status admin UI and provide status weights it occurred to me that if you delete a signup status in the settings UI, any existing signups with that status are left with bogus data in the {signup_log} table. Seems like we have two options:
A) Automatically set all the existing signups with the deleted status to status ID 0 (which means no status).
B) On the confirm form, you select a new status to re-assign all existing signups of the deleted status to. There could also be an option here for "None" which would set the status ID to 0 (just like option A).
Thoughts?
#1
Upon closer inspection, we really need Batch API for this, since the act of changing the status means we should invoke hook_signup_status(), and that could be expensive to do all at once. I don't think (A) is a good idea, and makes things weirder when we're invoking the hook that the status was changed (e.g. for signup_status_mailer). So, I went with (B) (and no "No status" option), and added the Batch API goodness to invoke the status change hook on each signup as we change the status.
On the confirm form, you're only presented with this reassign stuff if there are any signups in the selected status. If there are signups of the status you're trying to delete, you have to choose the new status to reassign. The form defaults to the old status and there's a validation callback to flag that as an error, so that if people try to mindlessly click through the confirm form, they won't accidentally reassign all the signups to a status they didn't really want to use.
Here are a few screenshots of seeing it all in action, too.
Thoughts?
#2
Going with just the reassigning of the statuses (B) makes the most sense.
I've tested the patch and it works as advertised. Forcing the user to make a conscious change by defaulting to the to-be-deleted status is a smart idea. I like it.
Marking as reviewed and tested.
#3
Cleaned up a little but of the redirect logic in here and committed to HEAD. Thanks for the review and testing.
#4
What's the purpose of not providing a "No status" option? For me, it would be more logical to default to that option instead of defaulting to an option that would cause an error.
#5
@roball: The point of defaulting to an option that's going to cause a validation error is to prevent people from mindlessly clicking through the confirm form. They're doing a destructive operation that can't be undone, and they *have* to think about it.
Setting a status back to "0" (no status) is basically a can of worms, since it makes various things like the status change hook invocation more complicated, which in turn complicates things using that hook, like the signup_status_mailer sub-module. So, let's move discussion of that over to #589246: Allow reassigning the status ID 0, not here. If we decide that's a feature we want to support, the patch for that issue can include whatever changes to this delete-reassign feature are needed, too.
Meanwhile, the bug described here is still fixed: when you delete a status now, there's no bogus data left in the system.
#6
Automatically closed -- issue fixed for 2 weeks with no activity.