CVS edit link for tomhung

I am writing to request CVS access that I might add the Webform Import module to the Contributed (contrib) modules repository.
Much like the Node Import or User Import modules, Webform Import allows you to import a set of Webform submissions from a delimited text file. Webform Import accepts all standard delimiters (comma, tab, semicolon, colon, pipe, period, and space). The Webform module currently allows you to export data to a delimited file, but does not allow for any method of import of data from external data sources.
Dependencies
· Webform module.
· Drupal 6.x
Related modules
·  Survey-Webform Migrate
CommentFileSizeAuthor
#19 webform_import.zip5.28 KBtomhung
#14 webform_import.tar_.gz4.96 KBtomhung
#12 webform_import_standards.patch8.39 KBquicksketch
#9 webform_import.tar_.gz4.5 KBtomhung
#11 webform_import.tar_.gz4.96 KBtomhung
#10 webform_new_help.png296.23 KBtomhung
#4 webform_import.png561.8 KBtomhung
#1 webform_import.tar_.gz4.03 KBtomhung

Comments

tomhung’s picture

tomhung commented
StatusFileSize
new webform_import.tar_.gz4.03 KB

code

tomhung’s picture

tomhung commented

I have searched google and drupal.org modules for similar modules and found none.

I have reviewed this module successfully with the "Coder".

tomhung’s picture

tomhung commented
Status: Postponed (maintainer needs more info) » Needs review

Please review the above tarball for addition to the Drupal 6.x CVS contrib modules

tomhung’s picture

tomhung commented
StatusFileSize
new webform_import.png561.8 KB

Screenshot

This is integrated into an Open Atrium feature. Yet OA is not required.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: +Module review

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

bcn’s picture

bcn commented

I was wondering what the status of this application is?

quicksketch (the webform maintainer) has stated that:

...after reviewing your code I'd be happy to link to your project from the Webform Project page. Note that the CVS application reviewers are going to want to see the code before granting your application, so you can just link to your application from here.

I've also performed a quick review of the code, and it looks okay to me as well. Is there anything else we can do to help move this along, as the code tomhung intends to release exposes some new and valuable functionality.

tomhung’s picture

tomhung commented

ping

tordrup’s picture

tordrup commented

Definitely +1 on this.

I'm very interested in seeing this application move forward. On more than one project, webforms has allowed me to quickly develop robust mini-applications. But not having a stable and reliable data import tool has added a ton of hours to these projects.

tomhung’s picture

tomhung commented
StatusFileSize
new webform_import.tar_.gz4.5 KB

Beta 2 - updated the help interface as well as added template csv creation.

This has bugs.. Newest version below

tomhung’s picture

tomhung commented
StatusFileSize
new webform_new_help.png296.23 KB

New help & download template screenshot

tomhung’s picture

tomhung commented
StatusFileSize
new webform_import.tar_.gz4.96 KB

CVS Account Application Checklist according to http://zzolo.org/thoughts/applying-drupal-cvs-account

Requirements and Other Modules

done - Make sure you read the CVS Application Requirements!
done - A lot of this same information is in the what to expect article but is often overlooked.
done - Describe how you looked for other modules and ANY module that may do similar things. I need to know you at least looked.

we looked extensively on drupal.org for similar modules. We contacted Webform module owner about the addition. He confirmed that there are no existing modules that import results into webform. He also expressed interest in linking to this module. (see http://drupal.org/node/733146#comment-2678100)

done - Are you leveraging other modules in yours? There are a number of stable API modules in Drupal. If applicable, you should use them, or tell me why you are not.

we are extending the Webform module. There are no other API's used or extented.

done - Is your module a feature that should be a part of another module?

we feel that this should be seperate from Webform as it is only needed by people converting existing datasets to webform. This is similar to how node-import is not included with cck or core.

done - Tell me that you have read these pages and link to them.

we have read and followed the advice CVS application process. http://drupal.org/cvs-application/requirements http://drupal.org/writing-secure-code http://drupal.org/coding-standards http://drupal.org/node/1354

Coding Standards

done - Follow the Drupal Coding Standards.

we have read and followed the Drupal Coding Standards (http://drupal.org/coding-standards)

done - Note that the Drupal Coding Standards include Javascript and CSS files (among others).

our module does not include any CSS or Javascript files

done - Use 2 spaces, not tabs.

our module uses 2 spaces for indenting (passed Coder Module)

done - See String Concatenations.

our module uses proper string contatenation (passed Coder Module)

done - Every text file that you put into the Drupal CVS repository should have the following at the top of the file: $Id$ In code files, this will be within a comment. This automatically gets changed by the CVS packaging system to something like to include, the date, your name, etc.

both the .module and the .info file have the proper $Id$ syntax included

done - All code files need to have a @file docblock.

the .module file has the proper @file comment included

done - All functions should have a docblock.

the .module file has the proper docblock comments included

done - Documenting hook implementations only need to reference the hook:

the .module file has the proper hook reference comments included

done - Run your module through Coder to help automate some of this. Note that you can change the options when running your module through Coder.

passed Coder Module

Drupal Module Basics

done - In the .info file, do NOT include the following, as they are added by the CVS packaging system.
.install files contain the following hook implementations if they are needed (and should not be in your .module file):

we removed version and project from our .info file

done - Any significant HTML output needs to be run through the theme layer.

theme_table() & theme_item_list() are used to produce html where appropriate

Using the Drupal API

done - Use the t() l() url() watchdog() drupal_set_message() when needed

t() wrap all translatable text, watchdog() are used where appropriate

done - The basic design pattern of input in Drupal, is filter output, not input. This means that do not alter input, but make sure that you filter any output to the screen that is user supplied. Know these functions and when to use them: check_plain() check_markup() filter_xss_admin()

we are not outputting user supplied text - webform & webform report are handling the output other than Error / Warning messages

done - Drupal has custom implementation of common PHP string functions to help support Unicode; see the functions in unicode.inc

drupal's unicode.inc does not have functions that apply to fgetcvs, therefore we are preg_replace() out of bounds unicode (see _webform_import_csvfieldtrim($v))

done - Read writing secure code and all its child pages! Seriously!

read and evaluated the procedures within this module - all check out, OK!

Third-Party Resources and Licensing

done - Do not put licensing information in your module anywhere. Anything that is put in the Drupal repository is automatically licensed at GPLv2.

we license this GPL. no 3rd part code or images are included

Attached

Attached is the lastest code for review.

quicksketch’s picture

quicksketch
he/him
commented
StatusFileSize
new webform_import_standards.patch8.39 KB

Overall this looks like a very valuable and well-written module. I haven't found it specifically mentioned anywhere, but it seems that this module is written to work with Webform 3.x, correct? When the project page is created, be sure to explicitly state which version it is compatible with (even if it's both versions).

Code review wise:
- hook_menu() should use %webform_menu instead of %node, otherwise you'll be able to visit node/[nid]/webform-results/upload on non-webform node types.
- Lots of commenting tweaks: All comments should be structured as sentences. A single line of documentation for the short description of the file or functions.
- The $form['instructions']['instructions'] text should be run through a theme function considering its extensive markup.
- Forcing dates to be in MM/DD/YYYY is a bit over-Americanized. But making the date format configurable is a bit sticky (see the code for expand_date()) and can be fixed after this application is approved.

Attached is a basic set of nit-picks (code formats and comments) that should be corrected, but on the whole this is an immaculate CVS application. Gets my +1.

quicksketch’s picture

quicksketch
he/him
commented

All of webform_import_form_validate() is unnecessary, since Drupal provides all of that validation built-into the FAPI (it's already impossible to spoof select list options or change a #type = 'value').

tomhung’s picture

tomhung commented
StatusFileSize
new webform_import.tar_.gz4.96 KB

here is a new version.

this has quicksketch's nits as well as support for ALL native webform components (except File Field which will not be added)

tomhung’s picture

tomhung commented

Bump!

I'm not sure what else I need to do. I have code review, recommendations, and a completed checklist.

Please push the button!

Greg

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Reviewed & tested by the community

I'd suggest waiting patiently or you'll upset the kind volunteers who review CVS applications. I'll mark this RTBC though, that should help separate this application from others that are in review.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work 
function webform_import_csvtemplate($node, $type) {
...
  $filename = $node->title . '_upload.csv';
...
    drupal_set_header("Content-disposition: attachment; filename=$filename");

$filename is not escaped in any way, but contains user submitted values. This can lead to forged HTTP headers.

  $form['header'] = array(
    '#value' => '<h2>Webform Import for "' . $node->title . '"</h2>',
  );

Same here. $node->title needs to be escaped prior to outputting it.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented 
  $form['header'] = array(
    '#value' => '<h2>Webform Import for "' . $node->title . '"</h2>',
  );

The string is not passed to t().

tomhung’s picture

tomhung commented
Status: Needs work » Needs review
StatusFileSize
new webform_import.zip5.28 KB

* @sun: security issues fixed
* @kiamlaluno: t() applied
* other small changes for optimization and ease of code read

tomhung’s picture

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

I am changing this request status back to "RTBC", since I have fixed the listed issues and have waited over 1 week for further comments.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs review
tomhung’s picture

tomhung commented

I would LOVE for someone to review these changes. Thanks to everyone that has helped out thus far.

Thanks
Greg

bcn’s picture

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

AFAICT this is good to go.

bcn’s picture

bcn commented

Overall this looks like a very valuable and well-written module.

I'm sorry to be so pushy on this, but what more can we do to convince those with the powers to give Tomhung CVS access?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

The code is fine; I have just some doubts about the following code lines:

    drupal_set_header('Content-type: application/octet-stream; charset=utf-8');
    drupal_set_header('Content-disposition: attachment; filename=' . $filename);
    print $csv;

Why the code uses that content type when the content is simply plain text?
After printing the CSV content, the code should invoke all the implementations of hook_exit(), and exit with exit().

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

bcn’s picture

bcn commented

@kiamlaluno-Thank you!

tomhung’s picture

tomhung commented

@noahb - http://drupal.org/project/webform_import

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

Issue tags: +Module review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Component: Miscellaneous » new project application
Issue summary: View changes