Default Status on Signup
| Project: | Signup Status |
| Version: | 6.x-1.0-alpha2 |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | sethcohn |
| Status: | needs review |
I am using Signup Status along with Signup and Signup Pay. Signup Status is the vital, missing piece to allow for me to track what signups have paid, and those who haven't. Given this use case, I have setup the following statuses.
Payment Pending
Paid
I would like to have Payment Pending be the default status so that, should a user not complete their PayPal transaction, then an administrator can make a decision about that person's registration. (I would also like to have their status change when the PayPal IPN returns a message that they have paid, but that's likely done with a Trigger and that's not in the scope here.)
I think I might just go ahead and try to patch the code to get this done and here's my work plan. Feel free to chime in if my methodology is off or you have any suggestions.
1. Alter signup_status_codes table code to include a new field "default" and the values should be 1 or 0. Note: This will be a global setting, not a node setting. (I know that there is movement on setting statuses per node.)
2. Alter _signup_status_status_form_element() to define the default. If there are no fields to "show on form" and there is a default set, do NOT put the default in a hidden field - security risk.
3. On submit of the registration, if a status value is not supplied, look for the default in the database: "SELECT cid FROM signup_status_codes WHERE default = 1. Then write the cid to signup_log.status
Regarding Singup Pay - outside the scope for this patch.
- Need to look into the Action module to see if the status can be changed once PayPal IPN says that payment has happened -- and also fire off an email.
#1
Ignore this comment - unable to delete.
#2
#3
Please find the attached 3 patch files uploaded with this issue. I followed the above plan and had some additional modifications along the way:
#4
I'm guessing you meant to say "(added 'default_status' field)" and not "(added 'default_status' table)" in point 3? Trying out these patches now...
#5
#3 is working for me. I tested update to add default column/field, enforced uniqueness of default, default is selected on signup form (if shown), and default is applied if no status is selected upon signup. Thanks!
Just wondering if a table column is the best way of going about storing the default? Why not use a variable? This might make it easier to store defaults for each node, too, since a variable name could be concatenated with the node id to distinguish it from the global default?
#6
Thanks for the patches. In principle, this seems like a valuable feature to add before the 6.x-1.0 release. However, there are a few obvious problems just from looking at the patches:
A) Please include all the changes in a single patch file, instead of separate patch files for each source file you touch.
B) I think the settings UI is confusing with checkboxes. If there's only one possible default, it should either be a set of radios in the table, or there should be a separate drop-down selector to choose the default.
C) The code needs to use spaces for indentation, not tabs.
D)
} else {on a single line doesn't match the Drupal coding standards.E)
function default_status()needs to use "signup_status" at the front of it for namespacing purposes (to prevent function name conflicts with other modules). In other words, it should be calledsignup_status_default_status().F) I agree with Oliver Coleman that it doesn't make much sense to have a whole new column in the DB to record which status is the default. There's only 1 default, so it seems a bit weird to have a column in the DB that has to be manually kept consistent like this. A site-wide default could just be saved in the variable system. If there were per-node defaults, that could be a separate table of (nid, cid) pairs, or it could be added to the {signup} table for per-node signup settings or something... But, a new column in {signup_status_codes} seems wrong.
G)
watchdog('signup_status', 'No default status could be selected.');I don't think we want that logged every time there's no default status selected. ;)If someone feels like re-working this patch to fix the above problems, please post a new patch and set this back to "needs review".
Thanks!
-Derek
#7
I've created a new patch that implements this using a drop-down to select the default status in the admin form and storing the default in a drupal variable. I've got a fairly particular set-up so although I think I've modified all the bits that need modifying I'm certainly not sure! I'm at least pretty sure I've modified the most important bits. ;)
This is the first multi-file patch I've created (in 3 years of drupalling!) so bear with me if it's not quite the right format (I followed the instructions at /patch/create to create a patch for a directory).
#8
Cool, thanks for the patch! A few problems from a visual inspection of the patch (haven't tried testing it yet):
A) There's no need to do this:
$codes = signup_status_codes();. Earlier in the same function (the very first line of signup_status_admin_settings_form(), in fact), we already do this:$signup_status_codes = signup_status_codes();. So, you should just iterate over $signup_status_codes.B) The default status if the variable isn't defined should probably be 0 (no status, like we have now), not 1, since there might not even be a single status code defined yet, and status 1 could be anything. Also, someone could easily delete status code #1, in which case if this variable was cleared, you'd get weird behavior.
C) A select box isn't going to work to add a new status that you want to be the default in one operation. I think a set of radio buttons for each row in the table (including the row for adding a new status) would be the best UI for this. I know it's a bit more tricky to code, but I think it'd be worth it.
D) There seems to be no way to say "I don't want a default at all". If we keep the select box, there should be a "- None -" option in there with 0 as the key. If we go the radios in the table route, we'd need another row at the bottom of the table with only the name and that radio (i.e. no delete link and no checkboxes).
E) 'signup_status_default' is a bit ambiguous for a variable name. I'd rather it was at least called 'signup_status_default_status'.
F) This description isn't very accurate:
+ '#description' => t('The status that will be automatically assigned to new signups.'),It's really more like:
"This is the default status that will be assigned to new signups which do not specify their own status (for example, if none of the status codes are visible on the signup form)."
That's a bit wordy, and could probably be tightened up, but that's the basic idea of how this feature would actually work.
Thanks for moving this forward, Oliver!
Cheers,
-Derek
#9
subscribe. I'll tackle improving this patch in the morning, as I need this for a project I'm working on right now.
#10
Ok, comments above are addressed in this patch.
I did _not_ implement Radio Buttons per C (from above), because the complexity involved solely to allow a single operation (ie create new status and also make it the new default in just one step) isn't worth the effort, IMHO. Implementing as radios not only requires adding an unsortable field for the 'extra' radio button to reside (to handle the 'no default' option), but increases the complexity of the form. A single select (with a 'no default option') is far cleaner and when you create a new status, hitting save will add that new field to the dropdown for you to pick, which is pretty intuitive to most users. Create + Save, Pick as Default + Save.
If Derek or someone else wants to spend the time to code up option C, please do, but it doesn't scratch my itch (since this is a 'setup once and forget about it' settings page for me and most others). I'd rather see this committed now than wait till someone who wants the radio buttons adds it.