This issue continues the now obsolete CVS Application for webform_civicrm, which has not yet been reviewed.
Here is the new git sandbox project for the webform_civicrm module.

Since I initially posted this module as a CVS application three weeks ago, it has received a great deal of attention from the Drupal/CiviCRM community, if you don't have time to read all 50 comments in that thread, here are some of the highlights:

  • 11 code commits by me, including a few bug fixes, some code cleanup, extra documentation, and about a dozen new features added.
  • Reviewed and tested by half a dozen developers
  • Now in use on a handful of production websites (including mine, of course)

The original project description is as follows:

CiviCRM is a powerful open-source contact management system, used by thousands of organizations around the world. Webform is also a very popular Drupal module that is widely in use. What I found myself doing over and over was creating a webform, adding fields like "first name" "last name" "phone number" etc. And then writing a little temporary module specifically to process that form, calling up the CiviCRM APIs, matching webform fields to contact fields, using hook_form_alter to set every field's #default_value for logged-in users, then saving the contact upon submission and creating a civicrm activity. Doing it once wasn't so bad, but after a while it gets pretty old. And that sort of thing is completely out of reach for someone who doesn't know PHP. So I decided to help myself out (and everyone else in that situation) by writing a module capable of doing all that and more for any webform. This module provides the missing link for anyone who uses CiviCRM and wants to collect information about, or track interactions with, their contacts. It allows you to:
-Automatically create webform fields that are linked to the CiviCRM database.
-Have forms auto-fill for logged in users.
-Have forms auto-fill for anonymous users if you send them a special hashed link through CiviMail.
-Automatically log activities when users fill out your form.
-It uses your CiviCRM default strict deduping rule to decide whether to update an existing contact or create a new one when the form is submitted by an anonymous user.
-It imposes no restrictions on how you style, rename, nest, or edit your CiviCRM fields, you can do anything with them that you could do with any other webform field.

Some comments I've received about this module:

  • "well coleman you are obviously enjoying yourself building this - and it is real good - like finding a new flavour of chocolate. Tested most aspects i think."
  • "This is a great module, just what I need."
  • "This module is fantastic and long overdue."
  • "It looks like the updated DB queries did the job. No more errors. Thanks again."
  • "This module is outstanding - we're very hopeful it will solve an ongoing problem for us."
  • "Thanks much for the quick fix, @colemanw"
  • "Holy moly - this is sensational! Just installed it and configured my first fully-integrated webform and it looks like it's working beautifully. "
  • "Coleman - latest version checks out A-OK on my end. This module really is the "glue" between Drupal and Civi that makes it all work nicely; no more messing with those awful HTML form snippets! Thank you SO much - the whole process is almost fun now. Almost ;-) Rock on!"

Comments

wyn’s picture

wyn commented

Add me to the list of thankful users of this tool.

Is the CiviCRM field availability to Webform currently limited to individual contacts? Specifically, I'm interested in adding fields to the form that are used only for Activities.

My envisioned implementation is as an application requiring an applicant's contact information but also a list of courses they've taken as prerequisites. Since more than one application can be submitted, keeping the fields in the Activity created by Webform submission seemed the best way to go.

However, I don't see a way to expose those fields to the Webform.

colemanw’s picture

colemanw commented

Hi @wyn. I think it would be good to open a feature-request issue about this for further discussion. However you should know that this feature would be by no means easy to implement, and goes beyond my original intent for this module. As far as my needs are concerned, all the information you collect in the webform is contained in the activity, in the form of a link to the submission record. So your use-case example (very similar to mine, I am also using it for application forms) can be achieved by creating plain-old webform fields to input prerequisites, and anyone viewing that activity in CiviCRM can find out what was entered simply by clicking the link to the submission. The only downside to this approach is that the data is not directly accessible in CiviCRM and therefore cannot be used for advanced search, smart groups, reports, etc.
-my $.02

petednz’s picture

petednz commented
Category: task » feature

Hey Coleman - great work on the various updates - have just checked out the latest nice touches. ping me if you are still wrangling your Contribution imports. Re this module - a thought about Groups - would be to separate them out from the Contact Fields since they are kind of hidden - yes I spent 10 mins trying to figure where to set them - any maybe the Enable Fields is a bit misleading. also yell out if you need some 'reviewed and tested by community' push

colemanw’s picture

colemanw commented
Category: feature » task

After opening #1075690: code review, Joachim has decided that he does not actually have time to review this module, so if anyone else out there has a few minutes to review it I would be most grateful.
There are quite a few people using it on their websites at this point, and being stuck with the "experimental" label makes their use of this module pretty inconvenient.
Thanks very much.

dharmatech’s picture

dharmatech commented

colemanw,

Great job with the module. This is only a code review since others seems to be testing functionality. Please consider the following issues/suggestions:

  1. Avoid using inline CSS styles. Let the theme decide how your markup will be displayed. Create additional theming functions if needed or provide CSS themes can use. Namely, all of the form prefix, suffix and description elements in webform_civicrm_forms.inc. Really is a distraction from the functional aspect of the code. webform_civicrm_forms.inc: lines 32-34, 50
  2. More descriptive high level code comments. Rather than 'Function to fill values', something like 'Fills values based on type of widget. Called when editing a form.'
  3. You may want to isolate the CiviCRM v2 API calls. v3 will introduce major changes in the API and if you isolate it now it will make it easier for you (or co-maintainer) to update.
  4. Braces provide clarity, please consider using them even for single line if statements.
    webform_civicrm_utils.inc: lines 85-87
  5. Exclude HTML from t() calls.
    webform_civicrm_forms.inc: lines 20, 50, 115, 335
  6. Requires a few brain cells to figure out what's going on. Maybe a comment would help or better yet, simplify.
    webform_civicrm_forms.inc: line 131
  7. StayConsistentWithCamelCaseForCiviCRM.
    webform_civicrm_forms.inc: lines 176, 185, 189
  8. $_GET['q'] should be sanitized.
    webform_civicrm_utils.inc: lines 458, 460
  9. Remove the semi-colon at the end of the if statement. Even though PHP doesn't care, it's a potential bug.
    webform_civicrm_utils.inc: line 284
  10. Did you really intend a $$? Avoid using reserved names as variables. Does it return anything other than $return?
    webform_civicrm_utils.inc: line 427

In addition:

  • Please consider writing documentation when the module becomes a project. The README doesn't count. :)
  • What is your commitment for supporting this module?
colemanw’s picture

colemanw commented

Wow @dharmatech those are great suggestions. I will work through them all over the next couple days. But a few comments/questions at this point:

  • All those damned inline styles were my (successful but messy) attempt to get the jquery.ui dialog to work right, which it doesn't out-of-the-box. But in hindsight it's a completely unnecessary feature, so maybe I should just get rid of it and stick with supporting popups or nothing. All it's doing is displaying the readme.
  • Would you please take another look at the $$ in #10? It is intentional (and documented) -- the input parameter $return specifies the name of the variable to be returned. So is the problem with the name of the variable?
  • How does one sanitize $_GET['q']?

To answer your questions:

  • The readme is pretty thorough, and I'm more than happy to add handbook pages as well and fill-in more FAQs as they get asked
  • This module is my baby, and I will most definitely be supporting it. Co-maintainers are welcome, but I'm happy to do it on my own as well. It's also feature-complete and working (the d6 version anyway) so unless we get into featureitis (which I'm avoiding, mostly, although I'm considering adding support for tags) I don't foresee much work being needed right now. The issue queue is bug-report-free!
dharmatech’s picture

dharmatech commented

colemanw,

You typically want to use check_plain(). See the page on writing secure code. l() will pass things through check_plain(), so maybe it doesn't matter in your case.

Yes, return $$return; just seemed a bit odd... an eye catcher. In the back of my mind, when reviewing code I look for things that might cause me grief when debugging at 2am. :)

Glad to see this module, it should be a huge help for folks.

colemanw’s picture

colemanw commented

Thanks again Dharmatech, I have gone through your list and made the following changes:

  1. Removed offending feature (opening readme in jQuery UI popup). I didn't really like it anyway
  2. Beefed up the comments quite a bit
  3. Waiting to learn a little more about APIv3 before acting on this suggestion, but it's good to think about
  4. Added braces for single line if statements.
  5. Excluded HTML from all t() calls.
  6. Added todo in comments for this one, I'm still considering the best way to organize all those checkboxes
  7. Fixed capitalization of CiviCRM
  8. No change needed. According to the source of l(), the whole string gets passed through check_url(), which should take care of sanitizing $_GET['q'] for us.
  9. Removed stray semicolon
  10. Works as intended, changed the variable name to be less confusing

Feel free to drop any more suggestions my way. I'm really looking forward to this becoming a "real" module!

dharmatech’s picture

dharmatech commented
Status: Needs review » Needs work

colemanw,

Looks great, just a few more:

  1. Exclude HTML from t() calls.
    webform_civicrm_forms.inc: line 39, 106, 109
    webform-results-submissions.tpl.php: line 37
  2. Braces provide clarity, please consider using them even for single line if statements.
    webform_civicrm_forms.inc: line 497

Sorry, just realized we should have been toggling the status back and forth between needs work and needs review.

colemanw’s picture

colemanw commented
Status: Needs work » Needs review

... and done.
Going back through all my code like this makes me start to nitpick myself. I just changed a few more things around to simplify and clarify, including merging two functions that were mostly redundant (so webform_civicrm_get_groups() is now gone, just a one-line addition to webform_civicrm_get_options() takes its place)

Regarding the use of t(), I re-read the docs on api.drupal.org and have done my best to follow the instructions, but two of the rules are somewhat at odds:

  • "try to put entire sentences and strings in one t() call."
  • "HTML markup within translation strings is allowed, but should be avoided if possible."

Leaves some room for interpretation when you've got html tags in the middle of a sentence. It also goes on to say that the proper way to handle <a> tags is to include them within t() but to use a placeholder for the url, so that's what I've now done. For the <em> tags, I think leaving it as-is is the best way to follow those two directives, since the html tags are trivial and the alternative is to mangle the sentence.
By the way, line 37 of webform-results-submissions.tpl.php isn't my code, it's the original webform template by quicksketch, and correctly follows the guidelines for handling <a> tags within t().

Setting back to "needs review" :)

dharmatech’s picture

dharmatech commented
Status: Needs review » Reviewed & tested by the community

Pretty sure "is allowed" means it won't throw errors. :) It's minor, but if you need to use <em> in t(), you can use %variable instead of putting the tag in the string. Setting to 'reviewed & tested by the community.'

sonicthoughts’s picture

sonicthoughts commented

+1 for an incredibly powerful module - please approve to drupal module ASAP. thanks Colman for the great work!

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Reviewed & tested by the community » Fixed

Git vetted user permission granted.

Thanks very much for contributing to the community. I did drop one patch into the issue queue.

I guess I should mention it here for those in this queue:

$Id$ is no longer used at the top of files since the git migration.
LICENSE.txt is no longer allowed in modules, as the packaging system fills that in.

Status: Fixed » Closed (fixed)

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

PA robot’s picture

PA robot commented
Component: new project application » module
Status: Closed (fixed) » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.