IXI: XML Importer is a module aimed at developers to help them when building sites where users will be uploading archives of XML files and those files need to be associated with another "owning" node. The module doesn't actually parse the XML - it leaves that to the developer through several simple hooks - but it does provide a framework for uploading and extracting files from the archive as well as some useful features like allowing the XML to be edited from the UI, or overriding some fields manually after they have already been parsed.

Project Page
https://www.drupal.org/sandbox/super_mario/2328781

GIT
git clone: git clone --branch 7.x-3.x http://git.drupal.org/sandbox/super_mario/2328781.git ixi

Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxsupermario2328781git

Manual Reviews

Files: 

Comments

PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsuper_mario2328781git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

super_mario’s picture

Status: Needs work » Needs review

Fix applicable issues from automated review.

Sagar Ramgade’s picture

Hi,

I had reviewed your module, here is my feedback :
a. Remove files[] array from ixi.info
b. Remove package information from ixi.info
c. Remove files[] array from ixi_features.info
d. Line no 65 ixi_reference/ixi_reference.module not sure why you are using $node->language instead of LANGUAGE_NONE constant.
e. Two READ files are there, remove one of them.
f. Remove files[] array from ixi_upload/ixi_upload.info
g. Remove package information from ixi_upload/ixi_upload.info
h. Remove package information from ixi_features.info
i. Use db_select instead of db_query and db_query_range so it compatibles to other other db engines.

super_mario’s picture

Hi, thank you for your feedback. Here are my responses:

a. Removed
b. I don't think this is correct. This module actually contains 4 different modules and I think it's helpful to have them grouped as a package.
c. Removed
d. The node's language has already been defined as LANGUAGE_NONE on line 48. If I wanted to change it in the future I would only have to change it in one place.
e. There are two separate arrays to read
f. Removed
g. see b
h. see b
i. It seems that using db_query for static queries is preferred due to better performance

bekirdag’s picture

Status: Needs review » Needs work
Issue tags: +PAReview: review bonus

Manual review:
1. Your folder name in git is 2328781, please change that to ixi
2. The includes files should be declared in .info file like:

files[] = ixi.admin.inc

3. You can use drupal_goto function instead of

$form_state['redirect'] = 'admin/reports/status';

4. You used old fashioned sql select queries, which i am not against at all, but someone sure will ask you to use db_select function, see https://api.drupal.org/api/drupal/includes%21database%21database.inc/fun...

Auto review

Please fix the issues in pareview.sh http://pareview.sh/pareview/httpgitdrupalorgsandboxsupermario2328781git

klausi’s picture

@bekirdag: ouch, almost all of that advice is wrong.

1) The folder name is determined by the git clone command and cannot be changed in the repo itself.
2) Only files containing classes should be listed in the info file.
3) do NOT use drupal_goto() in form submit handlers, $form_state['redirect'] is the correct way.
4) do NOT use db_select() for simple static queries, see https://www.drupal.org/node/310075

Leaving at "needs work" for the coding standard fixes reported by pareview.sh.

super_mario’s picture

Status: Needs work » Needs review

All remaining issues from pareview.sh are false positives.

naveenvalecha’s picture

Issue summary: View changes

Updated issue summary.

er.pushpinderrana’s picture

Issue tags: -PAReview: review bonus

Removing review bonus tag as @bekirdag added it, @super_mario you can add it, if you have done 3 reviews of other projects.

@bekirdag, I noticed your other reviews also, you are adding PAReview: review bonus tag everywhere, please don't do that. Only author of project will add this after doing 3 reviews of other projects.

bekirdag’s picture

@er.pushpinderrana sorry for the confusion, will do in the future.

@klausi thanks for clarification.

naveenvalecha’s picture

Issue summary: View changes

updated issue summary.

naveenvalecha’s picture

Status: Needs review » Needs work

@super_mario,
Thanks for your contribution.
Change the issue status after fixing the suggestions as mentioned by @klausi in #6.Also fix the automated Review by pareview http://pareview.sh/pareview/httpgitdrupalorgsandboxsupermario2328781git
Also I have mentioned the automated review link in issue summary page and also updated it.

You should first fix above issues first and get for coming in the admin radar you need do get review bonus by manual reviews of another 3 modules. This would help speed up the process and will put you on the high priority list, then git administrators will take a look at your project right away.

klausi’s picture

Status: Needs work » Needs review

super_mario already said that the automated review lists potential false positives, so please do a real manual review.

joachim’s picture

Instead of maintaining your own database table, would it not be possible to build on top of core's file field? That would significantly reduce the amount of code you need here.

naveenvalecha’s picture

Status: Needs review » Needs work

Hi @super_mario,
Thanks for your contribution.Nice module and also sub modules.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and the README Template.
Code long/complex enough for review
Yes/No: Follows the guidelines for project length and complexity.
Secure code
May be. Not reviewed complete module yet. If "no", list security issues identified.
Coding style & Drupal API usage
  1. Update the changes as mentioned by @klausi in #6 https://www.drupal.org/node/2337931#comment-9172793
  2. Also address on #14 https://www.drupal.org/node/2337931#comment-9188357
  3. ixi.install : Set these values of the variables a default value instead of setting on the hook_install.Full specific as @klausi addressed. Do not set variables in hook_install() with variable_set(), you can just use default values with any variable_get() call
  4. ixi.install : First check that whether the private filesystem path is set or not.
  5. ixi.module : Rename the function name _ixi_config_form to ixi_config_settings_form as it seems more good.
  6. ixi_upload.module : Change the pagecallback & page arguments of the 'admin/content/add/xml' path to be same as the 'admin/content/add/xml/upload'. No need to specify the redirect it will open the upload page callback same on the 'admin/content/add/xml'

This review uses the Project Application Review Template.

You should fix above issues first and get a review bonus by manual reviews of another 3 modules that would help speed up the process and make sure it gets on the review admins radar.

I got the Error during installation of modules so would you please fix this.Screenshot attached Due to this I was not able to test the module completely.

Thanks Again !

naveenvalecha’s picture

Screenshot as mentioned in #15

PA robot’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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