Ubercart Postal code lookup

Hi.
Some code clearing needs to be done. More info in here:
http://ventral.org/pareview/httpgitdrupalorgsandboxbendik1594840git

You should also be working in a version specific branch (ie 7.x-1.0), not in master.

Hi, manual review

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• Link to report http://ventral.org/pareview/httpgitdrupalorgsandboxbendik1594840git

@ jleiva

Seems like you missunderstood the sense of the "review bonus" tag, please read https://drupal.org/node/1410826
(btw, Pasting automated reviews into applications is not enough, manual reviews are required.)

Do not paste the full results into the issue, as giant ‘wall-of-wrong’ posts are extremely demotivating to applicants. Rather provide a link or attachment of the results.

As you can still edit and shorten your comment - please replace your text with the proper link

Hi bendikrb, looks like you have to add one space after switch and foreach, and maybe this will help you with the function error http://drupal.org/node/1354#functions

README.txt

• looks good to me - content is concise and explains what the module does. Possibly listing "obvious" requirements might be a boon; ubercart, for example.

uc_postalcode.info

• again, the dependencies list is half-filled. Listing ubercart would not be a bad idea.

uc_postalcode.install

• using direct DB writes to alter the module weight? I'd be tempted to use hook_module_implements_alter but this method is certainly standard.
• The comments for the hook_install are buried inside it. Moving them to the method comments would make a lot of sense.

uc_postalcode.module

• Unnecessary newlines throughout.
• Several variables declared away from where they are used.
• No comments on uc_postalcode_form_uc_cart_checkout_form_alter.
• Use of sprintf over DAPI:format_string, this doesn't guard against XSRF etc.
• Service returns in Norwegian when an address is not found, either it needs to be translated, configurable, or check against the response->status for the error messages
• Incorrect use of drupal_http_request: this function already has handling for param string building, so there is no need to use drupal_build_http_query directly.

To mention a couple of issues that will prevent this module installing at this point in time:

In uc_postalcode.info - the dependency on "ubercart" is invalid. This should be changed to for an example uc_cart, an existing core module in the ubercart package.

On line 187 of uc_postalcode.module (branch 7.x-1.x) - you will get a "Parse error: syntax error, unexpected ';'", because you've closed off the parenthesis' for the array, but not the format_string function. It should be:

return format_string('<span id="@id">@city</span>', array(
  '@id' => $id,
  '@city' => $city,
));

From the log it seems you accidentally introduced this bug through your last commit.

A less critical issue:

$element['delivery_postal_code']['#ajax'] = array(
  'callback' => 'uc_postalcode_lookup_delivery_callback',
  'effect' => 'slide',
  'progress' => $element['delivery_postal_code']['#ajax']['progress'],
);

This leads to the progress message of "Receiving shipping quotes...", which might seem a bit illogical when fetching the city. Another issue is that if you type in an invalid postal code, and get the message "Invalid postal code" added as a suffix to the field, then try again with a new postal code... the "Invalid postal code" suffix is not removed while a new query is ran. This leads to a message of "Receiving shipping quotes...Invalid postal code" displaying while it runs... which is potentially confusing to the customer, and somewhat problematic wrt. available space behind the field in most checkout pages.

Apart from that - it looks like a nice and handy module. Thanks for sharing! :)

You are most welcome.

I don't find the "Receiving shipping quotes" message illogical; changing your city (zipcode) will in most cases lead to a different shipping quote!

In a web shop that charge for shipping, it might not be that illogical... that is true. But it is becoming increasingly common to offer free shipping these days (and completely hide/remove the shipping-method part of the checkout). In that use-case it'd seem quite odd / out of place imo.

Don't forget that far from everyone shopping online are tech savvy or used to computers / online shopping. You'd be surprised how little it can take to go from the mindset of "Intending to complete the checkout" to...

"Why are they calculating shipping when they say it's free?! Will I be charged extra? Hmm.. I don't know what's going on here, so I'll abort and look for the product in a physical store (or different online store) instead".

As much as 1 out of every 3 shoppers drop out during checkout, on average, according to the statistics... and the details do definitely matter when it comes to the overall impression / the customers sense of security. I'm not going to call the customers "stupid", but I wouldn't leave anything to chance when designing a checkout-process. ;)

How about making the message configurable, so that it can be adapted to the sites specific needs? Seems like the sensible (and not too hard to implement) solution, so long as you also keep it translatable for multi-lang support. You could offer a choice between "Receiving shipping quotes" and for an example "Looking up city...", or a text field for entering your own message. :)

I could look in to hiding the field suffix while ajaxing, but I don't think it's that big of a deal really.. :)

Thanks. I think that'd be a good addition, as it'd come across as more professional / polished to the customer.

I disagree; your module adds functionality outside what Ubercart provides by default, completely altering the premise of the original message. It is no longer merely used in combination with the shipping pane - it is triggered upon filling in a postal code, and appended as a suffix to that field, regardless of whether the shipping pane is present or not. Hence it would be completely within reason to replace the message and adapt the behaviour to fit the new context. ;-)

If you'd like to keep compatibility with whatever the Ubercart project sets the "standard message" to in the future, you could provide this by default but offer an option to override it through this module settings. Tick a box to replace it with a translatable "Looking up city..." for an example. The default behaviour is fine for stores who charge for shipping, while the override would be a lot more suitable for stores offering free shipping (or have no physical shipping at all). These are after all pretty common these days.

I'm just trying to suggest an improvement that'd make your module suitable for a broder set of online stores. It'd enable it to cover more than one use case, and hopefully gain a larger user base in time. Whether you feel it's worth listening to is obviously completely up to you. I appreciate the work you've put into the module nonetheless. :)

Your code contains a few spacing/punctuation standards issues. See http://ventral.org/pareview/httpgitdrupalorgsandboxbendik1594840git.

You appear to be using an awkward method for theming the form elements and configuring the AJAX. The normal way is really quite easy. See the AJAX reference and form reference for details.

Use '#prefix' and '#suffix' to put a form element in a div, not a theme function.

Use '#theme' to theme a form element. It will get called automatically that way.

Your code only works for checkout. There are many other places where Ubercart uses addresses. If your code applied to the uc_address entity/object, it would work in lots of places.

When creating an order, the following errors popped up:

• Notice: Undefined index: delivery_postal_code in uc_postalcode_process_address_field() (line 66 of /var/www/html/drupal_test/sites/all/modules/uc_postalcode/uc_postalcode.module).
• Notice: Undefined index: #default_value in uc_postalcode_process_address_field() (line 80 of /var/www/html/drupal_test/sites/all/modules/uc_postalcode/uc_postalcode.module).

The city did appear. However, it appeared next to the postal code. This is kind of distracting. I'd like to see the city displayed on it's own line, as you normally would on an address form. This might require the combination of a hidden field to store the value and a markup field to display it. You could also just have the city be a normal text field and updating the postal code updates it.

Could the postal code also be used to determine the state or province?

Did you give up on publishing/maintaining this project, bendikrb?

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1606634
Project 2: http://drupal.org/node/1886184

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

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