[D7] Commerce So Colissimo Flexibility

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

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.

I have test this module, and it works fine for me.
I can add prices on shipping services with a nice javascript tool !
I can see all services if available on checkout.
Good module for me, with nice features on back-office.

You could clean and complete your project page (with HTML tags in place of *).

You include an external librarie in your module : infobox.js

The good practice will be to :

• to not import automaticaly this librarie on your module
• to make sure your module is fonctionnal without this lib
• to complete on install.txt file with the possibility to add an external librarie with a short manual (how install an external lib)

Add Scald Commerce Product review

The module is working fine and from my perspective, ready to be promoted.

manual review:

1. "variable_del('socolissimo_api');": all variables defined by your module need to be prefixed with your module's name to avoid name collisions.
2. Suggestion: the module name is a bit long, shorten it to commerce_socolissimo?
3. "Fail to load google map": all user facing text must run through t() for translation.
4. theme_commerce_socolissimo_pane_review(): empty function, please add a comment why it is empty.
5. "@new SoapClient($socolissimo_api['url']);": why do suppress errors with "@" here? Please add a commment. That should be avoided whenever possible.
6. commerce_socolissimo_flexibility_rate_new(): why not simply entity_create()?
7. commerce_socolissimo_flexibility_rate_save(): why not simply entity_save()? Also elsewhere whenever you use entity_get_controller().
8. "module_invoke_all('commerce_socolissimo_flexibility_service_status_insert', $shipping_service);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php

Haven't reviewed everything, but found no critical application blockers, so this looks RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to dman as he might have time to take a final look at this.

Code is good, well structured (and huge), and the activity here in the queue has been positive, so I agree we can say there are no blockers to a full project.
I can't run-test it because it's quite a set-up

Some notes however.
* I notice a few places in the theme files where you have coded actual user-facing text. eg commerce-socolissimo-point-details.tpl.php

 print t("Shipping under !day days", array('!day' => $element['#delivery_delay'])); 

... Now, although it's good that you've used t() there, so there is still a mechanism to override it, seeing text in a tpl file often means that that should be admin-configurable. Because believe me, clients will want to change it!
It's not a big deal for this project, as I'm pretty sure that anyone who uses it will want to be doing their own theming etc anyway - just a thing I noticed.
Heck, the fact you are using theme tpls at all (and documented them!) is more than we can expect usually!

Just a suggestion. In my experience, remote services are a weak point in any testing, as I've often seen sites go up and then fail on these because either the live host is firewalled or the remote service is unavailable for unpredicted reasons.
see that you have already put that in a try{} so I think you are good there.
What *I'd* do is add
* a watchdog log just before you go out to the world in commerce_socolissimo_api()
* a timeout parameter to your SOAP call
... but that's just because I learned to be paranoid. That's what I always insist the guys put in so we can at least see WHY the site suddenly stopped responding one day.

Bravo for even catching filter_xss on the return message!

Documentation throughout is great, clear README etc.
Good use of Drupalisms for JS and CSS inclusions etc.
It looks like you've got solid eror handling and things built in to appropriate places.

Like I say, I can't do a line-by-line, but I can say this code is of the highest quality (as we must expect from a commerce gateway) and looks like a great addition.

-----------------

Thanks for your contribution, theo_ !

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.