ShippingEasy

welcome,

It seems to me that you've included 3rd party code (libraries) into your module:

• ecommerce-webservicelib
• apilib

3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
If you have concerns about using the Libraries API please explain what and why you're including it.

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

Please take a moment to create a README.txt that follows the guidelines for in-project documentation.

While waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxshippingeasy1478832git

You can also get a review bonus and we will come back to your application sooner.

regards

Okay, if these libraries are developed and maintained by you, there should be no problem with that!

BTW, actually only one account per person is allowed on drupal.org (your name indicates kind of group)
If there are more people developing behind the shippingeasy useraccount, please consider to create one account per developer and add them as co-maintainers. As this process is about making sure that we only give users the permission to create full projects that have a good understanding of drupal practices it is in our interest that only the account of the main developer will get this permission. Otherwise it's okay.

Hello,

We're now using different accounts as you suggested. nebojsae is the account of the main developer who will need the permission to create full projects. He added the README file, fixed most of the code style errors in the extension code and the code should now be in the proper branch.

A question about the code style. Almost all of the CodeSniffer errors remaining are in the of our libraries. Can we ignore those errors, since the code is not related to Drupal in any way?

Thanks.

Hi there. I set the status of this ticket to "needs review", since we're waiting for a reviewer to go through this extension. Hope that's OK.

Hi,

Manual Review :

1.The use of private methods or properties is strongly discouraged, use "protected" instead in all your class files.
2. It seems that you are using "?>" tag at end in all your files. Please remove all the "?>" tags at end.
3. Please use appropriate drupal functions for all string functions that you were using. ex: str_replace('*', '%', check_plain(arg(5) in shippingeasy.inc.
4. Please dont create link markup by your own.Please use l() to create it.
ex: My Account in shippingeasy.inc.
5.Also try to avoid using HTML inside t(). ex: line:84 in shippingeasy.inc.
6.There are huge number of errors in validation.so Please PAREVIEW your codes.

Thanks

Hi,

Thank you very much for the review. We'll try to resolve the points you raised and get back here when done or if we have additional questions.

Regarding the code style, please note that most if not all of the CodeSniffer errors are in our internal library that the Drupal module uses. We'll double-check to be sure. I think it's fair for non-Drupal code not to be subject to Drupal's coding standards. Do you agree?

Thanks again,
Jozef

Hi there,

The latest iteration of the code has been pushed to the repository.

Items 3, 4 and 5 have been resolved. Items 1 and 2 are in our internal non-Drupal library code. Regarding the validation errors, as I explained in my previous reply, they are also in our library code. The only validation error that remains in the Drupal module code is the following:

WARNING | Format should be "* Implements hook_foo()." or "Implements | | hook_foo_BAR_ID_bar() for xyz_bar()."

/**
 * Implements hook_menu(). (uc_order.module)
 */

I would appreciate your help in resolving it. Is there anything else we need to do to get approval?

Thanks!

Manual Review:

1) You can use theme_image($path, $alt = '', $title = '', $attributes = NULL, $getsize = TRUE) function for image tag in shoppingeasy.module.
2) Remove unnecessary comments as at line 59 and 67 in shoppingeasy.inc.

Automated Review:

Please try to resolve automated review.

I'll admit that for myself, the atmosphere of wording in certain posts seem a bit less than motivating - if you know what I mean. However certainly we all understand the frustrations involved with the time & effort it takes to complete the vetting process. But what mayhem it would be if we did not conform properly. :)

Also, you may want to take a look at the Review Bonus program - if you review 3 projects, you can get your module reviewed by the "top dogs" substantially quicker.
So moving right along to my review...

Very nice platform your team has created. It truly does make shipping so much less tedious and awkward, so much more comfortable within UberCart. Can't wait to scrap all of my carrier accounts, and use a single site API.
I do wish this was available on Drupal 7.x, as I've already migrated to it and won't go back myself. I'd suggest looking into that, as UberCart has a stable 7.x version available. However I've created a 6.x sandbox for myself, added UberCart & all to manually test this.

Not sure what the problem is with https://www.shippingeasy.com/ajax/register - but regardless of browser, it simply displays a "Please Wait" button image on registration submission - and the account isn't made, as I cannot login after waiting over 10 minutes. So that is really preventing me from fully testing the module. Chrome is showing that the ajax form is indeed being sent.

4. Almost all of the CodeSniffer errors remaining are in our libraries. Can we ignore those errors, since the code is not related to Drupal in any way?
       Given that these API's ('apilib', & 'ecommerce-webservicelib') are capable of providing other developers basis for developing new modules to extend the capabilities of your module & website - they really should meet Drupal Coding Standards (as should all published code). However, based on your website it would appear you use this code for ALL of your platforms, not just Drupal. So no, I don't believe it would be fair to ask that you rewrite your entire code-base to a standard that is humanly readable & properly documented for Doxygen, as a requisite for obtaining full-access for the module. If you intend to provide documentation on drupal, you really should though.
8. See previous list-item. Spend a few hours on it, not a big deal really. I personally keep 2 copies of my project, 1 that I can read myself (I don't use spaces, or documentation, and I over-use camelCase), and another that can be read by other users adhering to the Coding & Security Standards.
9. WARNING | Format should be "* Implements hook_foo()." It means exactly that, and so literally it should read (modified):
   

   /**
    * Implements hook_menu().
    */
   function shippingeasy_menu() {
     // This hook implementation is for: (uc_order.module)
     global $user;

11. See previous list-items.
12. See previous list-items.

To reiterate comment #6

, in exclusion of your API libraries, the following changes should be made...
After line-by-line review of your code, I would like to mention that you have done a good job on following the Drupal Coding & Security Standards. For instance, you are properly using check_plain, and that is extremely important. The following are items that I've identified personally. I highly recommend the Code Sniffer PEAR extension (It has very good documentation for installation and usage, on various platforms & setups).

3. Please use appropriate drupal functions for all string functions that you were using
   • shippingeasy.install #42(strlen): $string .= $alphabet[mt_rand(0, drupal_strlen($alphabet) - 1)];
   • shippingeasy.module #529(date): ->addDate(format_date($details['current_time'], 'custom', 'Y-m-d'))
   • shippingeasy.inc #105(strtolower): $courier_name = str_replace(' ', '_', drupal_strtolower($value));
   • shippingeasy.inc #401-406:
     

      $billing_first_name =     drupal_strtolower(str_replace('*', '%', check_plain(arg(5))));
         $billing_last_name =   drupal_strtolower(str_replace('*', '%', check_plain(arg(6))));
         $billing_company =     drupal_strtolower(str_replace('*', '%', check_plain(arg(7))));
         $shipping_first_name = drupal_strtolower(str_replace('*', '%', check_plain(arg(8))));
         $shipping_last_name =  drupal_strtolower(str_replace('*', '%', check_plain(arg(9))));
         $shipping_company =    drupal_strtolower(str_replace('*', '%', check_plain(arg(10))));

4. Please use l() to create ... link markup
   shippingeasy.inc #84 (a href)
   

   '#description' => t('To start using ShippingEasy, you will need to connect your store to a ShippingEasy account using an API key. The API key can be found at the bottom of the My Details tab in the ') . l(t("My Account"), "https://www.shippingeasy.com/my") . t(' area of the site.'),
   

Credit to: Novalnet

For the sake of easily moving forward with this

I've created a patch file including the above changes. It is attached below.

I was unable to create an account, and therefore was unable to fully verify your logic-flow or end-user-usage. As I said though, the code indeed is well written, so I would expect you have thoroughly debugged and verified this kind of operability; especially considering your team has already begun marketing.

best of luck, and thanks for the module - it truly is a great concept & implementation.
p.s. Make a 7.x version for us afterwards! :P

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Added info about main-dev