Synopsis

This module provides functionality for integrating Drupal Commerce with the DPD shipping system.

Commerce DPD provides the following integration:
- Create and insert shipments into DPD.
- Retrieve labels for a shipment.
- Retrieve parcel number for tracking a shipment.
- Rules integration to turn commerce orders into DPD shipments.

Link to sandbox:
https://www.drupal.org/sandbox/_antoniofazari/2307525

Clone the repository:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/_antonioFazari/2307525.git commerce_dpd

Requirements

Commerce
Commerce Order
Commerce Customer
Rules

Comments

_antonioFazari’s picture

Issue summary: View changes
_antonioFazari’s picture

Issue summary: View changes
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/httpgitdrupalorgsandbox_antonioFazari2307525git

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.

_antonioFazari’s picture

Status: Needs work » Needs review
benjy’s picture

Overall the module looks good, nice work!

  1. This isn't actually a valid hook.
    /**
     * Implements hook_settings_form().
     */
    
  2. I don't mind explicit if that was your goal but you have a lot of the defaults listed in your form declarations.
        '#size' => 60,
        '#maxlength' => 128,
    
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
    
  3. Why not wrap this in Simpletest and put your assertions there? https://www.drupal.org/simpletest
    commerce_dpd_test_shipment_creation()
  4. This could be replaced to use url()
    $url = variable_get('dpd_api_url') . '/user?action=login';
    to
    $url = url(variable_get('dpd_api_url') . '/user', array('absolute' => TRUE, 'query' => array('action' => 'login')));
    
  5. Consider drupal_json_decode(), although beware it does json_decode($json, TRUE) so you'll end up with an array instead of an object.
    $data = json_decode($response->data);
    
  6. You should check $response->code first to make sure that the request succeeded.
        $response = drupal_http_request($url, $options);
        $data = json_decode($response->data);
    
        if ($data->data->geoSession) {
    
  7. example.json doesn't exist in the repo?
      if (variable_get('dpd_test_mode')) {
        $path = drupal_get_path('module', 'commerce_dpd') . '/example.json';
        $data = file_get_contents($path);
      }
    
  8. Check the response code first, data won't be set if it failed.
      $response = commerce_dpd_request('POST', $headers, $url, $data);
      $data = json_decode($response->data);
    
  9. I don't quite follow this, if they weren't authorised we authenticate again to get a new session but then it's not used here?
      if ($response->code == 401) {
        // This user is unauthorised, create a new geo session token.
        $response_data = $data->error;
        watchdog('commerce_dpd', 'Error occurred: %error', array('%error' => $response_data), WATCHDOG_INFO);
    
        // Authenticate again to get a new geo session token.
        commerce_dpd_authenticate(TRUE);
        watchdog('commerce_dpd', 'Authentication: Attempting to get a new geo session token.', array(), WATCHDOG_INFO);
      }
    
  10. Typo
      // Autheticate with DPD and get a session token.
      $geo_session = commerce_dpd_authenticate();
    
  11. Would be nice to add a configure link to your info file.
    configure = admin/commerce/config/shipping/dpd
  12. commerce_customer_shipping might not always exist and this will throw a fatal error.
      // Get the shipping details from the order.
      $order_wrapper = entity_metadata_wrapper('commerce_order', $args['commerce_order']);
      $shipping = $order_wrapper->commerce_customer_shipping->commerce_customer_address->value();
    
  13. Readme looks good, would be nice to add some more setup instructions. Eg, what event to put the action on for the normal use case. Might be obvious to most but can't hurt.

Good work :)

PS. (to the admins) - It's hard work reviewing without Dreditor, we should get the modules posted as patches so we can do reviews much easier.

stefank’s picture

The module does work as expected. There is still and spellcheck error in code ./commerce_dpd.module:278: succesfully ==> successfully.

Maybe it is worth to add some more information on the project page, as how to setup the module. To test shipment, there is nowhere a link to the test page.
Also adding a link to dpd site(register) and let users know that they need to have an username and password, before using the module.

Thanks and keep up the good work.

k_zoltan’s picture

Status: Needs review » Needs work

Looks like this module need work based on the latest reviews.

Ar you still working on this project?
If not please help others works in maintaining the issue queue by closing this ticket.

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.

_antonioFazari’s picture

Status: Closed (won't fix) » Needs work
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.