Hi Coleman,
Thanks so much for this project!

I'm trying to give the module a bit of tough love by looking at it, running it and running it through coder_tough_love after downloading it from git yesterday.
1. I believe best practice is for the project name webform_civicrm_integration to match the filename of the .info and .module files. I think you could either shorten the human readable name to Webform CiviCRM, or if you want the longer and more accurate Webform CiviCRM Integration then you should use webform_civicrm_integration.info etc.
2. When editing a field at node/nid/webform/components/nid&destination=node2Fnid%2Fwebform it would be good to have a Cancel button instead as well as the Submit button.
3. In the attached patch I've made a number of changes to your files including:
- fixing spaces in very many places around concatentation operator .
- adding { } where you didn't have them on single statements in if / else structures. NB: I think I missed some.
- adding spaces after , in a few places
- removing trailing spaces at the end of lines in a few places
4. I added External ID as an additional field since I sometimes need it.
5. I added // $Id$ to some of your files where it was missing.
6. I believe I added {} around Drupal table names in a few SQL statements (not positive as I did it for all tables by mistake and then removed for civicrm tables).
7. I'm not sure it is a great idea, but I removed the final ?> from webform-results-submissions.tpl.php. If you want to leave it in then it would still be a good idea to make sure there are not trailing blank lines
8. I changed a few tabs to two spaces, eg webform_civicrm_utils.inc and in a few places changed the indents to make them consistent.
9. Changed PHP constants to upper case in a few places (eg NULL instead of null).

I hope this helps.

CommentFileSizeAuthor
webform_civicrm.patch26.01 KBjoe.murray

Comments

joe.murray’s picture

joe.murray commented

I just noticed that $Id$ has been deprecated by move to git. Please ignore those elements of patch.

colemanw’s picture

colemanw commented
Status: Active » Needs review

Hello Joe, I appreciate your tough love, especially the love part... it's good to see people taking up this module and putting it to use. I trust your testing of functionality went well. As for the tough part, I didn't apply your patch because so much of it was fixing the concat operator, and you're a little too far ahead on that one... while D7 standards require a space before and after the dot, this is a D6 module, and so I think I'll stick to the D6 standard as specified (no space between a string literal and a dot). Although there has been a great deal of arguing about it on d.o. so it seems that one opinion is just as valid as the other.

The other suggestions were good, and I have incorporated them into the code. Another advantage of patching manually is that I caught a few more things that you didn't, (including a misspelled variable name which would cause a minor API call to fail, yikes!).

I have also added your feature request of external_id, although it required a little more coding than what was in the patch (processing as well as defining the field). I haven't tested it, could you?

joe.murray’s picture

joe.murray commented

Great. I'll test shortly.

I was using the coder_tough_love approach to spacing around the concat operator since I thought it reflected accepted D6 practice. While I have a small preference for the extra space ;) my big thing is just to follow some agreed practice.

The module is working well for me and I will be putting it into production on a site today. I have only tested adding some non-custom CiviCRM fields to a webform and creating Activities that are either a default or custom Activity type. So I haven't played with Groups, Tags, or prepopulating the Webform. Matching existing contacts, and creating unmatched contacts worked for me.

I had one bug that I wasn't able to reproduce that I'm pretty sure was due to me changing the code without restarting the session. What happened was that after changing the label of a field so that it matched one that was already there, then deleting that field, both of the similarly named fields were deleted.

FWIW, my change re: external_id was saving and displaying the data properly after only changing $fields in webform_civicrm_get_fields. However, I tried adding user_unique_id to $fields as well and that didn't work, and I wasn't sure what was required in addition. I was guessing that CiviCRM returns external_identifier by default but not user_unique_id, and I don't really require the latter.

A few questions:

Can you expand on the comment around line 377 (my version) of webform_civicrm_utils.inc:

// The order of these fields is important: see function webform_civicrm_contact_match

I wasn't clear on a quick review what impact a changed order would have on the processing in the latter function. As this is one of the easiest and most beneficial ways to add functionality to the module, it might be nice to explain how to add fields to the existing order. I'll try diffing your version against mine to see where things need to be added for processing a field and add some commentary on how to do that. Then I'll add some documentation on it. Would you like that to go into the README.txt, a different doc file, or perhaps have a book page on d.o setup for this?

I'm not positive, but I think that the v3 API for CiviCRM is stable enough now that this module should be using it.

It seems like it might be nice to add support for scheduling a followup when a webform is submitted. For example, if the form requests the org to do something like update some information or send out something, then it would be good to create a task with a due date for a staff person as well as send that staff person the email via webform. Have you got suggestions for a UI for this if I manage to find time to build it?

In terms of feature requests, I wonder if it might be possible to avoid hard-coding the default strict individual contact type match rule, and adding optional support for matching on criteria for other contact types (like Organization - I don't think Household is nearly as important). I was thinking of adding Organization to list of fields supported but didn't because I realize it isn't properly supported.

colemanw’s picture

colemanw commented

Re deleting fields: This module doesn't care about deleting webform fields, whether it is a civicrm-linked field or not, you can delete it and this module won't respond to that event, interfere, or be affected at all. Keeps things nice and simple. So I doubt that anomaly had anything to do with webform_civicrm.

There are 3 things involved in supporting a new CiviCRM field:
-add it to the $fields array and give it the properties you want (in this case, external id needs to be of type hidden, otherwise you're opening your form to mishaps and abuse)
-check you haven't messed anything up for civicrm_contact_match(). It is basically constructing a multidimensional array out of a flat one (filing all those fields into the appropriate table: civicrm_contact, civicrm_address, civicrm_phone, etc.), and counting on certain keys to show up first to trigger the switch (so as soon as it gets to street_address it switches to filling in $params['civicrm_address']). All those "key" keys are commented as such. Also, in the case of hidden fields, you need to fill it in after the fact (also with civicrm_contact_match()) because you might know who the person is after they submit the form but not before.
-If there's anything special about that field, you can add some additional alterations to _webform_civicrm_webform_component_form_alter(). In this case I just tacked on to the rule we were already using for civicrm_contact_id

Filling in the field (even though it is hidden) on the form isn't a bad idea since some people might be expecting it, so have added the capability to do so for hidden fields (you have to alter the #value, not just #defualt_value). Will commit that in a little while.

Supporting organizations is an interesting idea. I'll have to give that some thought, and also find out if that feature is something people want. Maybe start a new issue for that feature request and see if it gets any attention.

I'm not familiar with followup reminders in CiviCRM. Can you tell me more about your idea and how it would interface with that system as it currently stands in Civi?

v3 API is not ready yet, and it also isn't shipped with any currently released version of CiviCRM. My plan is to switch to it for D7, but leave D6 with v2 so that all versions of CiviCRM 3 are supported, not just 3.4.

thomaslucas’s picture

thomaslucas commented

the baby works smooth, great module! if there is one feature I would like to see (and I think it's more about webform than the implementation) is to be able to get in the admin email the url were the form was filled... because I use the webform as a block and knowing if it was filled in that or these url is basic for me...

colemanw’s picture

colemanw commented

@thomaslucas: You should file that request over in the webform issue queue. Unfortunately, knowing your current url is not something that Drupal is particularly good at. But it can at least make a reasonable guess based on $_GET['q'] and aliases of that path.

thomaslucas’s picture

thomaslucas commented

Thanks Coleman! I have been looking around and I found something that (at least for now) works, just adding:

%server[HTTP_REFERER]

to the email template (webform -> e-mails -> click on the email address and then edit the template) seems to do the trick! Please confirm if you (or anyone else) have a chance

Thank you for the module again,

colemanw’s picture

colemanw commented
Status: Needs review » Fixed

I believe all the bitty stuff in this issue has been committed, so marking as fixed.
New issues should be opened for other feature requests and bug reports.

Status: Fixed » Closed (fixed)

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