Uses the auspost postage assessment api with the Commerce Shipping module to provide postage rate estimations on physical items.

http://drupal.org/sandbox/jhesketh/1851514

git clone --recursive --branch master http://git.drupal.org/sandbox/jhesketh/1851514.git commerce_australia_post

For Drupal 7

Files: 
CommentFileSizeAuthor
#11 Commerce_shipping_Australia_Post.png26.33 KBcog-axis

Comments

vineet.osscube’s picture

Hi,
I suggest you to take a look at this page:

http://ventral.org/pareview/httpgitdrupalorgsandboxjhesketh1851514git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Manual Review:

1) There is no hook_uninstall to remove your custom variables from database like: commerce_australia_post_api_key, commerce_australia_post_rates_timeout, etc. in admin.inc

2) Please don't add LICENSE.txt, it will be added by drupal community after completing review process and full project release.

Joshua Hesketh’s picture

Hello osscube,

That's a neat tool, thanks for that.

I've fixed up the issues you've pointed out and some from the automated tool. The remaining errors seem to be false positives or about formatting. How pedantic do we need to be about the formatting? To fix some of the errors would reduce the readability of the code.

Cheers
Josh

revureviewsite’s picture

Hi

Josh you need to fixed all formatting issue as well to get approval.

Thanks
Alok

stijn.blomme’s picture

Status:Needs review» Needs work

Hi,

overall

- Your git branch still uses master, this should be changed to 7.x-1.x
- Try to write a little more documentation on your module page.
some suggestions:
- list the dependencies for this module
- you need an api key for this module to work => provide a link on the module page
- give some basic installation instructions on how to configure shipping methods.

first code review

- You should indeed fix all the encoding issues thrown by the automated test bot.
try to use module_load_include in stead of require (commerce_australia_post.module line 8)
http://api.drupal.org/api/drupal/includes!module.inc/function/module_loa...

I will try to do a deeper code review once all these issues are fixed

Joshua Hesketh’s picture

Hi guys,

Thanks heaps for the feedback.

I've fixed all of the issues that the coder module highlights and updated the project page.

Let me know if I've missed anything.

Cheers,
Josh

Joshua Hesketh’s picture

Status:Needs work» Needs review

I've also branched into 7.x-1.x although I couldn't remove the master branch from drupal.org.

jhaskins’s picture

You can find info about how to make that the default branch & get rid master in steps 5 & 6 at http://drupal.org/node/1127732.

One potential minor issue I noticed: it appears that you are calling $order_wrapper = entity_metadata_wrapper('commerce_order', $order); twice within commerce_australia_post_build_rate_request (lines 12 & 93). Is there a particular reason for the second one that I'm missing?

Aside from that and a few coding style issues that I won't bother mentioning (the previously linked automated review tool will show you all the ones I saw plus all the ones I missed), I really don't see any issues.

klausi’s picture

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

mitrpaka’s picture

One minor item to add:
- According to Drupal Commerce's development standards commerce_australia_post.admin.inc and commerce_australia_post.json.inc should be replaced under /includes -folder

Please see http://www.drupalcommerce.org/developer-guide/development-standards#stru...

Joshua Hesketh’s picture

Thanks for the feedback.

- I've fixed up the extra $order_wrapper.
- I've removed the master branch from the repo (I couldn't find how to set the default before!).
- I've moved the includes into their own folders.

The module passes all tests done by the coder module. The ones picked up by the pareview are difficult to fix without harming the readability of the code.

Cheers,
Josh

cog-axis’s picture

StatusFileSize
new26.33 KB

Hi Josh,
I have tried to test your code. It is not working on my machine. It is giving me empty page Shipping when I checkout.

I'm using this address:
Australia
Zip: 2176
City: Sydney
State: NSW

Same Zip code: 2176 used in module configuration.

Please also try to fix these http://ventral.org/pareview/httpgitdrupalorgsandboxjhesketh1851514git, it increases code quality and maintain Drupal standards.

Good luck.

Cheers!

swim’s picture

Hey Josh,

Just a quick one but I think you missed this variable on uninstall.

<?php
variable_get
('commerce_australia_post_rates_timeout', 0);
?>

There was also this implementation of the Aus post API http://drupalcode.org/sandbox/kimpepper/1347922.git. Not sure how relevant this is, did Aus post update their API recently?

Chairs,

Joshua Hesketh’s picture

Hi guys,

Thanks for that hapax, I've added it to the uninstall.

cog-axis: Did you enter a zip code on the configuration page? I've patched a few things with the post codes this morning that checks they exist before sending requests to auspost. Basically no postage options are returned if it doens't work. This way you can still select non-auspost options. However if no options are presented commerce should show "Please supply all of the required information requested above to reveal your shipping options." (which it does for me). How have you configured your checkout?

kimpepper's sandbox was a copy of the UPS commerce module (which I also based this off) but they didn't get very far. I spoke to Kim before starting and they had abandoned the module.

Cheers,
Josh

kingswoodute’s picture

Subscribe

Awesome work guys. On behalf of other Aussie Drupal users, thanks heaps. Are you going to make this into a module?

tunaman’s picture

Category:task» feature

Second kingswoodute. Thanks for your work on this much needed feature. I would be very grateful it was released as a module as well.

Joshua Hesketh’s picture

Category:feature» task

Hi guys,

Thanks. However this is already a module! :-)

This thread is because I want it turned into an official project rather than just my sandbox. If you want to check it out though take a look here: http://drupal.org/sandbox/jhesketh/1851514

Cheers,
Josh

tripper54’s picture

Hi Josh,

Thanks for you work on this much needed module!

I'm in the process of reviewing - at this stage it looks really good. I'll have a few suggestions but no release blockers I think.

Just one question to help me complete the review. I have installed the module on a Commerce Kickstart install (with the demo content), registered and received an API key, and configured a couple of Auspost shipping services. I then disabled the flat rate services that are included with kickstart.

Now when I check out the system tells me "No shipping rates found for your order. Please continue the checkout process."

Is there something else I have to do to get the Auspost shipping services to work?

thanks!

Joshua Hesketh’s picture

Hi tripper54,

I haven't tested it against the kickstart install so I can't be certain. However the shipping options only appear if auspost finds a way of posting the parcel. To do that you need to fill out the postage address field first. The module then uses that to calculate the postage and displays the options.

Let me know how you go.

Cheers,
Josh

tripper54’s picture

Status:Needs review» Needs work

Hi Josh,

Thanks for the info. I had another look at it - it seems the products need to have size/weight fields for the shipping options to display. Works well now! On the settings page you have a note "If products do not have physical dimensions and weights associated with them, the estimates will not be accurate". Perhaps you should change this to say Auspost shipping will not be available to products with no dimensions/weights? Also it's probably worth adding something to the README.

Ok, the review!

Duplication etc:
No concerns here. Drupal Commerce sorely needs an Australian shipping quotes module!

Manual testing:
Installation and configuration was straightforward, instructions in the README.txt clear and easy to understand (see note above).

As mentioned above, I tested this on Commerce Kickstart. The configuration was in the expected place, and clear and easy understand.

I went through the checkout process several times with various quantities of product, and the shipping quotes seemed in line with what I'd expect to pay at the post office.

Automated test:
Your code is still throwing notices at http://ventral.org/pareview/httpgitdrupalorgsandboxjhesketh1851514git . My understanding is that these are not RTBC blockers, but you might want to take a few minutes to fix them up, if only to stop reviewers nagging you about them.

Manual Review

Overall I found the code to be well written and well commented.

All files:
* doxygen blocks should begin with /**
* inline comments should use // not /*

commerce_australia_post_json.inc :

* line 9: your doc block should include a description of the $order param
* line 16:

<?php
$shipping_profile
= $order_wrapper->{$field_name}->value();
?>

It looks like you don't use $shipping_profile. Perhaps delete this line?

* there is a block of commented code from line 26. You should remove this.

* line 101:

<?php
$attributes
['from_postcode'] = variable_get('commerce_australia_post_postal_code');
?>

This is user input, so I guess you should add sanitation to this - check_plain perhaps? Likewise line 123:

<?php
$options
= array(
   
'headers' => array('AUTH-KEY' => variable_get('commerce_australia_post_api_key')),
  );
?>

commerce_australia_admin_inc:

* You might want to put some validation on the postal code field on the settings form (not a blocker, just a suggestion).

commerce_australia_post.module:

* line 95: UPS -> Australia Post ;)
* line 198: this function requires a docblock.

And that's it! Well done on an awesome addition to any Australian DC developer's arsenal. I'm happy to mark this RTBC if you can fix the above.

Joshua Hesketh’s picture

Hi tripper54,

Thanks for such a good extensive review :-).

My apologies for the delay, I've finally found some time to fix the items you've addressed (at DrupalCon Sydney nonetheless).

I haven't fixed all of the codesniffer items as it is difficult to do so without affecting the readability. At least in terms of a lot of line-lengths being difficult to truncate due to overly verbose (but readable) variable names.

Cheers,
Josh

tripper54’s picture

Nice one Josh. I'll have a look and hopefully RTBC shortly. Also at DrupalCon - looking forward to the CI session in 10 minutes!

Joshua Hesketh’s picture

Bump.

It's a pity we missed each other at DrupalCon tripper54!

klausi’s picture

You need to set the status to "needs review" if you want to get a review.

mitchell’s picture

Assigned:Unassigned» mitchell
Status:Needs work» Reviewed & tested by the community

I read through the comments and glanced at the code. It appears that Joshua Hesketh has gotten the module to an acceptable state with the help of many kind reviewers, and the code is confirmed to work.

Setting to rtbc. For the remaining code improvements, please use the project's issue queue.

mitchell’s picture

Assigned:mitchell» Unassigned
Status:Reviewed & tested by the community» Fixed

Thanks for your contribution, Joshua Hesketh!

I updated your account to let you 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:

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

Thanks to the dedicated reviewers as well.

Status:Fixed» Closed (fixed)

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