Hi,

I looked a bit into implementing cloning of paid events and I think it actually was this module that was causing the problem (it clobbered the old webform data when it added the two obligatory fields). Patch seems to work for me, but not tested.

Thanks!

CommentFileSizeAuthor
#13 cloneable.patch5.39 KBAndyF
#11 cloneable.patch3.22 KBAndyF
#10 cloneable.patch3.22 KBAndyF
#7 uc_event_registration-1055154.patch8.52 KBRob_Feature
#5 cloneable.patch2.75 KBAndyF
#2 cloneable.patch2.74 KBAndyF
cloneable.patch3.12 KBAndyF
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AndyF’s picture

AndyF CreditAttribution: AndyF commented

PS I was thinking it might be an idea to give more distinctive names to those components?

AndyF’s picture

AndyF CreditAttribution: AndyF commented
FileSize
cloneable.patch2.74 KB

Sorry, dunno why my editor chose to use tabs. Cleaner patch attached.

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented
Status: Needs review » Closed (won't fix)

Right now, this module assumes that all nodes that are created have blank webforms...the only reason they wouldn't have blank forms is if you were using node clone module. So, basically, this is incompatible with node clone module.

Honestly, it probably will be incompatible for the forseeable future (ie. it's not a priority)...if I decide to support that at some point I'll address it in this issue.

---EDIT---
Oops, didn't see the patch...

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented
Title: Cloning clobbers the webform » Node Clone Compatibilty
Category: bug » feature

Oh, snap, I just realized you attached a patch for uc_event_registration to enable this. Sorry, I'll mark this as to be reviewed....sorry about missing that.

AndyF’s picture

AndyF CreditAttribution: AndyF commented
Title: Node Clone Compatibilty » Cloning clobbers the webform
Category: feature » bug
Status: Closed (won't fix) » Needs review
FileSize
cloneable.patch2.75 KB

Added new components to beginning of array, and removed stray curly bracket.

AndyF’s picture

AndyF CreditAttribution: AndyF commented
Title: Cloning clobbers the webform » Node Clone Compatibility
Category: bug » feature

Sorry, didn't mean to clobber the settings!

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented
FileSize
uc_event_registration-1055154.patch8.52 KB

Your patch plus changing default field machine names. Please test

//I shouldn't do both in this issue, but....
Thinking about what changing machine names will do to existing installs (not that it should stop it, since it's in beta and its a good idea...)

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented

On event creation I get
warning: array_unshift() [function.array-unshift]: The first argument should be an array in /sites/all/modules/uc_event_registration/uc_event_registration.module on line 32.

AndyF’s picture

AndyF CreditAttribution: AndyF commented

sorry, posted the last patch in a hurry to get home, didn't test, bad me! It's a small mistake, will quickly change now.

If the machine names change then I think there needs to be a hook_update() function to change the name of the field names for existing users. I'd be ok adding that tomorrow if you'd like?

AndyF’s picture

AndyF CreditAttribution: AndyF commented
FileSize
cloneable.patch3.22 KB

Here's a quick fix for the patch. I'm at home, so can't test at all. Will follow up tomorrow. Cheers!

AndyF’s picture

AndyF CreditAttribution: AndyF commented
FileSize
cloneable.patch3.22 KB

Ooh. just noticed a missing parenthesis *cough*, night night.

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented
Status: Needs review » Needs work

AndyF: I shouldn't have introduced the name changes in this issue or this patch. Bad idea on my part.

Please go ahead and get this patch for node clone up to snuff and then post the patch for name changes and hook_update changes over on #1047506: Rename the field quantity

I won't be able to issue a new release for a couple weeks, though, so I'll thank everyone for their patience ahead of time :)

AndyF’s picture

AndyF CreditAttribution: AndyF commented
FileSize
cloneable.patch5.39 KB

Here's the cloneable patch, without a name change to fields. I'll post a patch for that hopefully later today on the other thread. Thanks.

AndyF’s picture

AndyF CreditAttribution: AndyF commented
Status: Needs work » Needs review
scubaguy’s picture

scubaguy CreditAttribution: scubaguy commented

I applied the clone patch and it worked in that the webform fields were cloned, however they are not being displayed when you view the event.

AndyF’s picture

AndyF CreditAttribution: AndyF commented

Thanks for testing scubaguy. The webform displays for me (I've cloned six paidevents and they all work properly). I've also just tried a fresh Drupal install with only the minimal modules required, and the clone still works properly. Do you have any custom modules that might be interfering with the webform display? It's strange that the fields cloned properly, but the form doesn't display.

scubaguy’s picture

scubaguy CreditAttribution: scubaguy commented

there are no custom modules installed and the extra forms fields display on the nodes where I inserted the extra fields manually into the database. I also compared the working form field set database entries to the non-working ones and I don't see a difference. I'll keep digging and report back. Let me know if you think of anything else to try.

scubaguy’s picture

scubaguy CreditAttribution: scubaguy commented

I figured out the problem. The cloned node had "0" set for the stock amount. It works great. thanks for the contribution.

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented

AndyF: Can you reroll this against the current HEAD? Since the field names changed it breaks stuff.

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented

Actually, scratch that. Went ahead and rewrote it and it's currently in HEAD. I'll push out a dev version for people to test. Please test and report back.

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented
Status: Needs review » Fixed

Marking fixed. Please test the dev version

AndyF’s picture

AndyF CreditAttribution: AndyF commented

Works for me, thanks.

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented

AndyF, did you review the actual code to make sure it retained your logic? Just want to make sure I didn't miss something. If it works properly I think I'll release this as a 1.0 version.

AndyF’s picture

AndyF CreditAttribution: AndyF commented

Yep, looks good. Some minor minor formatting points

  1. On line 45 it's probably best to indent 'unpaid|Unpaid', to be inline with "paid|unpaid\r\n" . on the line above.
  2. Lines 84-93 are indented once too many times.

On to more meaningful things, I'm happy with the performance for myself, but I'm aware that it's not as efficient as it could be. When writing it I figured it was premature optimization to be worrying about it (and it will certainly not be a problem for me on my site). But it's true that if you have both a large component array, and the quantity and payment status components are at the end of that array, then the array will be fully iterated three times (once to find the quantity, once for the payment status, and once for finding the next component ID). Personally I think that even if you do hit those two conditions, it's still unlikely that you're going to be doing enough paid_event cloning for it to be an issue, but just wanted full disclosure :)

Thanks

Rob_Feature’s picture

Rob_Feature CreditAttribution: Rob_Feature commented

AndyF: thanks for the update. I'm new enough to module development (and advanced php for that matter) that I'll lean on you (or others) to think about optimization.

I'll clean up formatting and release a 1.0. If you want to optimize that section of the code, feel free to post a patch in a new issue. Thanks!

AndyF’s picture

AndyF CreditAttribution: AndyF commented

Thanks, I'll leave it as is unless some flags it as a problem :)

Status: Fixed » Closed (fixed)

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