The ICA Survey module is a module that implements an API for building surveys. By default it comes with some modules that can be used to start building surveys without writing a single line of code, displaying them to users and capturing the results, but, more importantly, it also comes with a large number of hooks other modules can implement to define new question types (currently the module comes with open questions, multiple choice questions and checkbox questions), new methods for displaying results (by default the module simply displays all answers to all questions in a table, but it also comes with an extra module that aggragates the results so administrators can see how many times a single answer was chosen, for example). All of these hooks are documented (see the file doc/hooks.html).
The ICA Survey module defines a page where surveys can be created / modified, a page where administrators can view the results, a page that displays a survey to users and a page that is displayed to users when their answers to a survey are saved.
Unlike current methods for creating surveys in Drupal, which, as far as I can tell, all rely on making site builders use the form builder API to build forms that just so happen to be surveys, the ICA Survey module has been designed from the start to build surveys according to an extremely simple, straightforward concept: surveys are series of questions that users must answer. The answers are stored in a table and modules that build on the ICA Survey module can process and display in some sort of sensible fashion.
The module was built because we, at the "Expertise Centrum Systeem Ontwikkeling" at the ICA Faculty of the HAN Vocational College in the Netherlands, needed a simple to use framework for building surveys. The ICA Survey module has been used to build a questionnaire that sends out recommendations to users based on their answers. Using current survey tools in Drupal would have made this fairly difficult. Using the ICA Survey module all we had to do to analyze answers and send out an e-mail message with recommendations based on these answers was to implement the ica_survey_results_saved hook.
The module can be found here: http://drupal.org/sandbox/rhmtts/1175318
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | ica_survey-coder.txt | 7.15 KB | klausi |
Comments
Comment #1
aspilicious commentedYou should read the improved coding standards. [google drupal coding standards]
And there are functions with no doc blocks.
.po files are obsolete these days. Translation should take place in localize.drupal.org.
files[] should only contain files that have a class or interface in them. You don't have any of those. So you can leave that out.
Remove
Comment #2
aspilicious commentedComment #3
rhmtts commented* All issues the coder module came up with have been fixed.
* All functions have doc blocks.
* All .po translation files have been removed. I suppose when this becomes an "official" module I can add the translation data to localize.drupal.org.
* All files[] lines have been removed
Comment #4
rhmtts commentedComment #5
tim.plunkettGot your email, will try to get to it soon. There is a huge backlog, sorry for the wait.
Comment #6
rhmtts commentedNo problem, just knowing it didn't get lost in the void is enough :-)
Thanks!
Comment #7
klausi* what does "ICA" mean? why this module name? Ah I see, your faculty name it is.
* Git release branch missing, see http://drupal.org/node/1015226
* README.txt missing
* "Implementation of hook_menu" should be "Implements hook_menu().", see http://drupal.org/node/1354#hookimpl
* you could move the admin form to a ica_survey.admin.inc file
* you should use simpletest for your test cases, see http://drupal.org/simpletest-tutorial-drupal7
* "// extract data for the new question, if any" comments should start capitalized and end with a "."
* array formatting: the closing ")" should be on the same indentation level as the line that started it.
* module, line 927: what is this ";" for?
Comment #8
rhmtts commented- We use ICA as a prefix for all modules we develop ourselves to avoid namespace clashes. I'd be perfectly happy to drop it. There currently exists a project called Survey but it's obsolete.
- Git release branch created.
- README.txt moved from doc subdirectory to main dirctory.
- All "Implementation of hook_xxx" changed to "Implements hook_xxx"
- Admin functionality was moved to ica_survey.admin.inc.
- The tests have been rewritten to use simlpetest
- Comments have all been capitalized.
- All code has been reformatted.
- "Module, line 927 : what is this ";" for?" - I'm sorry but I don't know what you mean. I don't see any loose ; anywhere?
Comment #9
klausiComment #10
rhmtts commentedI'm very sorry about the coder module style issues. Since entering the approval process I've taken to religiously running coder on all code I write, but not, apparently, on the module that is in the approval process itself. Sorry for wasting your time like this.
All style issues have been now fixed, except one:
+180: [minor] When labelling buttons, make it clear what the button does, "Submit" is too generic.
Because in this specific case "Submit" is exactly what it does (it submits a survey).
As far as the other issues you raised are concerned:
> wrong git branch name: "7.x-1.0-rc1" should be "7.x-1.x". Use git tags for specific versions
Fixed.
> lines in README.txt should not exceed 80 characters
Fixed.
> ica_survey_uninstall(): function is empty, so remove it
Fixed.
> "// The page callback will set the title.": Comments should be on a separate line
Fixed.
> ica_survey_theme(): hook doc block is wrong.
What does this mean, exactly? If you mean to change the doc to read "Implements hook_theme()." that has been fixed.
> ica_survey_show_form(): do not add extra ";" at the end of foreach() loops or if constructs. They are not needed.
The coder module does not complain, and taking them all out is a lot of (manual) work that I'd rather not do if I don't have to :-) Is this really such a big problem?
> ica_survey_show_form(): "if ($is_even) {" is checked twice, why?
Once to actually do what it's supposed to do, namely to set a class for even and odd rows. The second time it's checked to toggle it. I agree it's verbose but at least it keeps things that don't belong together separate (IMHO).
Comment #11
rhmtts commentedComment #12
doitDave commentedHi, a quick glance at your repo:
There is still a branch named 7.x-1.0-rc1. You should really delete this branch and make sure to only work on the major release branch to avoid confusion. Same for the contents in the master branch, you better empty that branch and replace its content with a short readme like described in http://drupal.org/node/1127732 (step 5).
Review of the 7.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Manual review:
As for your recent comments:
So why not just name it "submit survey"? Background: Someone might alter your form. This actually happens quite often, btw ;). What if someone else comes along with a botton like "save"? Then your user sits in front of the form and thinks "ok. submit or save?". Out of question: "save" would also not be good style, but I'm sure at least you would want to provide the more sophisticated button in such cases ;)
Counter question: Is running a regex such a big problem? Actually you would not even need a regex but a simple search/replace pattern here ;)
I think this implementation is up to you. But you would have your code far cleaner with
$is_even = !$is_even;without any different result. Just to mention it ;)Comment #13
rhmtts commentedOK. Master branch has been cleaned up, rc1 branch has been deleted, all coder issues fixed except for the warning about files not ending in a new line because the files do, in fact, end in newlines so I'm not sure what it's complaining about.
Comment #14
doitDave commentedHi,
yes, automated checks look good and I can confirm that there are newlines. So formal issues seem fixed, please give me (or anyone else) some time for an in-depth manual review (as there's much code). :)
Comment #15
doitDave commentedOK, some first results after a test run and some code diving:
Maybe you would like to check on these few initial points first and do some more tests? If I can be of assistance, just let me know :)
Edit: Personal addition, maybe a question of individual taste: You are introducing 6 modules in one package with cascading dependencies. To deactivate the entire package, it takes three cycles. For deinstallation another three, six in total which I personally find really much. I would recommend to make sure that this logic is without alternative, which means, does it make any sense at all to activate only the basic module? Is there any value in a survey if not even "question type open" is activated anyway? In other words: If there's no real good reason to split a module into submodules (which I cannot judge after the first look) that should be avoided.
Comment #16
rhmtts commented* The admin section is now in admin/config/survey. None of the groups under config listed on http://drupal.org/node/549094 seemed to fit, so I created a new group.
* Fixed.
* Error messages have been added for when users create a question without a question text, which is what seemed to have happened in your example.
As far as the 6 modules are concerned: while you're correct that a survey without a single question type has no value, it should be possible to deactivate all question types, which is why even "question type open" is not part of the core module.
Looking over the six dependent modules again and with, hopefully, fresher eyes, I still don't see any one for which enabling by default makes sense.
I understand your distate for this setup and I even share it, but in this case I see no way around it (other than creating a basic survey configuration page where question types, some of which come from separate modules, can be enabled or disabled, which I personally find distasteful, if only because it leads to more complicated code).
Comment #17
osmanCode Review
Review of the 7.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #18
rhmtts commentedIt's great that a new version of pareview.sh catches more spaces and comment capitalization issues than previous versions (I'm 100% sure the last time I ran this through pareviews.sh all it did was incorrectly complain about missing newlines at the end of some files)
Of course I'm perfectly willing to address real, significant issues such as the ones raised in #16, but flagging things like "More than 2 empty lines are not allowed" as an error is, IMHO, ridiculous.
Could someone please review the contents of the module first before running it through yet another automated style tester with constantly revised settings. Since submitting this module more than half a year ago I've had to make it conform to four different style testers. Only once did someone actually look at what the module does. I'm about ready to give up on it, to be honest.
Comment #19
raynimmo commentedI feel your pain, remember though that you can run the automated tester at http://ventral.org/pareview yourself and address issues when they come up.
The online PAReview service that patrickd setup seems to be pretty up to date with the latest standards so you should maybe follow the output from that. He has also created a module in his sandbox so that you can use your own server to do the code review.
Comment #20
rhmtts commentedRight, so I ran pareview on november 24th and everything was fine, except the new line at the end of the file issue that was decided to be a false positive (see #14 above).
It's great that pareview.sh has since been changed to catch more, but it's unrealistic to expect people to keep up with every single change. And looking over the "issues" it comes up with there's nothing there that I think requires fixing before an actual human looks at the code to see what it actually does.
Comment #21
raynimmo commentedThe PAReview script is still a sandbox project and thus is liable to change and/or false positives. Klausi is constantly updating the script to include new checks and refine previous ones. Just a quick glance at his commit log for it shows that there has been numerous commits since November 24th. In essence this means that it will now detect errors that it previously overlooked.
He also seems to be transferring a lot of tests over to Drupal Code Sniffer as the PAReview script works in conjunction with that and a host of other testing modules.
I understand your frustration, my own projects review also has recently shown errors that where previously not reported on by the scripts but we have to take this opportunity as a learning experience and try not to get too upset by the evolving test results.
You should really use the online service as it really is a good quick way of checking your code if you do not have the appropriate testing environment installed.
The current implementation shows quite a large number of errors and warnings against your current codebase, see yourself at http://ventral.org/pareview/httpgitdrupalorgsandboxrhmtts1175318git-7x-1x. You should ensure you have thoroughly read and understood the Drupal Coding Standards and the Doxygen and comment formatting conventions whilst referencing the lines highlighted by the online review script.
Many reviewers will not attempt a manual review unless the code is clean and tested already through one of the automated testing environments. This ensures that they only have to search for security issues and common errors within function declarations.
As well as the 'More than 2 empty lines' issue, the tests are flagging errors such as incorrectly formatted functions, missing return parameters, missing data types, incorrectly formatted comments, incorrectly positioned comments, missing comma's in multiline arrays and incorrectly nested if/else statements to name but a few.
I feel that Osman was quite right to set this to 'needs work' in comment #17 and I have done the same.
Comment #22
rhmtts commentedRight. I have better things to do than to conform to the constantly changing whims of an automatic code validation script.
Comment #23
jthorson commentedJust soliciting process feedback ... I understand your frustration, but would you have preferred it if you received no feedback for a few months, and then suddenly a manual review; as opposed to the frequent coding style update comments?
I'm not being sarcastic here ... just trying to get a sense of whether the current experience is better or worse than what applicants were going through roughly 8 months ago; with full understanding that neither is ideal.
Comment #24
rhmtts commentedThe issue is not that there are frequent coding style update comments per se, the issue is that the coding style *changed*. At one point this module passed the automated coding tests. Then all of a sudden it didn't (which was, imho, incorrect, too).
I can't keep fixing code because some flaky automated testing script changes all the time. Once or twice to test if you're serious: ok. But once that is done, please look at the contents of the module (which is what this should be all about, after all - coding style is just cosmetics and can be changed easily).
I'm all for contributing our custom modules, which are written to be as generic as possible, back to the community and so is my boss, but all this red tape is taking up too much of our time and I can't justify that, neither to my boss nor to our clients.
So in this sense, I feel the process has gotten worse. In the old way it took a long time for someone to take a look at your code but at least the feedback, once you got it, was meaningful. The feedback you get now is, first of all, meaningless and second of all so frequent it's frustrating rather than helpful.
My suggestion to make the process less off-putting would be: run it through an automated style tester ONCE, then, if most, if not all style issues have been fixed, have a real live human test the module, give feedback about the contents of the module and at some point, based merely on the contents of the module, decide whether it's accepted or not. Only then should any last remaining coding style issues be resolved, with the full understanding that once they are, the module goes in.
Just my 2 cents.
Thanks for your interest.
Comment #25
jthorson commentedThanks rhmtts, that's the sort of feedback I was looking for. :)
Comment #26
avpaderno