[D7] Addressfield Autocomplete

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

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.

Your git clone link cannot be accessed by others. You should use http protocol for git clone command -
git clone http://git.drupal.org/sandbox/adammitchell/2126989.git

You shouldn't use master branch but 7.x-1.x instead. See:
https://drupal.org/empty-git-master

Hi Adam,

You should have a install file to remove the declared variables.
Also, it is good to add dependency in your info file if you are extending / using a contributed module.

I am getting JS errors;

Uncaught ReferenceError: o is not defined
and:
Uncaught ReferenceError: fd is not defined

I had set up a Google Maps API V3 key at https://code.google.com/apis/console/?noredirect because the link on your sandbox page (https://developers.google.com/maps/signup) can be confusing - might be worth updating. I created a new key, but maybe I didn't do it right? Step by step instructions would be handy because Google has been changing the API console a lot.

OK, so I followed the tutorial and am using the correct key but am still getting JS errors. I am using Chrome and jQuery 1.7. I tested with and without compressing JS (in Performance settings) and also tried jQuery 1.5 and 1.8 without effect. Attaching screenshot in a moment.

It's the Gmap module. I disabled it and now your module works beautifully. I do need my Gmap module, and I suspect others may too, but you can just add that as a bug that needs to be fixed. So long as people know about it I think that's fine.

Master Branch

It appears 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.

• Also rename the branch 7.x-1.x-dev to 7.x-1.x
• You have a fair number of style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxadammitchell2126989git, you should try to fix most of those.
• I think Enter one manually should be run through t(), as with all user-facing text.
• The CSS file should follow drupal code standards, available here: https://drupal.org/node/1886770.

Looks like a very useful module, setting to needs work for the number of issues.

----
Top Shelf Modules - Crafted, Curated, Contributed.

going to agree with kscheirer here. The style issues aren't a huge deal though. And the Gmap conflict shouldn't hold it up ... just note it on the module page until you have a fix for it.

Very good to hear. This is working. I still get some JS errors but they don't affect the functionality:

Uncaught ReferenceError: o is not defined

So.... RBTC but maybe check on the JS issues (Chrome)

please remove console.log from js file , and there is some line which exceed 80 character , use drupal coding standard.

This is an interesting module I want to contribute myself.

There are some minor things to be fixed, which I'm sending a patch:

• Do not add .css on the .info file: if you add .css in the .info it will be added on every page. The correct way is via the form array (the way you are doing in code) or drupal_add_js(). Read: https://drupal.org/node/542202#stylesheets
• Minor fixes on some comments.
• There is no need for using if ($widget['type'] == 'addressfield_standard') { because you have only 1 widget. Moreover, after the if you return $form, which may be undefined. I've fixed it removing the conditional.
• In function addressfield_autocomplete_field_attach_submit() there is $items = field_get_items(...). This may lead to a bug at the next line - foreach - because field_get_items() may return FALSE. I've fixed it adding a conditional before the foreach().
• There are many coding standard mistakes at the JS file, for example not using lowerCamelCase for variable names. I've fixed only 2 of these variables.
• A couple trailing spaces somewhere. Fixed.

The patch may be found at your issue queue: https://drupal.org/node/2156297
The attached patch can be applied using git apply patchname.patch. This will keep the correct author information.

Hello Adam,

there are still some minor mistakes:

• Some found by pareview

http://pareview.sh/pareview/httpgitdrupalorgsandboxadammitchell2126989git

• README.txt

I think you want to delete the line with the link to instructions to get the google API key: http://drupalcode.org/sandbox/adammitchell/2126989.git/blob/51292c5488d3...

• git

- Make sure you have the same user configuration across your machines. I see in the module repository you sometimes using the username Adam Mitchell, other times you are using adammitchell.

- For each commit fix only one issue. It will be better for later understanding what is going on in the code history. I see a commit you fixed 2 different issues.

- Use the command git apply. This will preserve the patch's author information. Contributors love getting credit ;) As you can see here you are listed both as committer and author of the patch I've sent. Read more at: Advanced patch contributor guide

I have many suggestions for this module and I'd like to talk more about it with you. Let me know if you are available anytime for a chat on IRC.

One of my suggestions is to not use this complex JS. You can instead use a very small JS to integrate with this: http://ubilabs.github.io/geocomplete/

This project looks good to become a full project. Marking it as RTBC.

Hi,

The field now has the wrong help text - it has some html tags in it...

"Please enter the address the <strong>event takes place at</strong>. We will use this to add a pin to the map of events."

Automated review:
addressfield_autocomplete.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
142 | ERROR | Whitespace found at end of line

.js file:
for statement doesn't loop at line 61
for(var country in settings.available_countries) break;

'gmapSettings.controltype == 'None' ? true : false' can be simplified to 'gmapSettings.controltype == 'None'' at line 48
disableDefaultUI : gmapSettings.controltype == 'None' ? true : false,

In general, there is inconsistent whitespace usage in the js. In some places there are spaces around the conditional if and else clauses. In other cases, this isn't true. Please put a space around the conditional statements, similar to how you would do in PHP.

See the section on Behaviors at https://drupal.org/node/171213. No need to pass Drupal and jQuery into the closure. Drupal is available from the global namespace.

(function ($) {
Drupal.behaviors.myModule = {
  attach: function (context, settings) {
    // Do something
  }
};
}(jQuery));

.module file:
I don't see where you end up using the drupal_static pattern, even though it gets called in line 165.

Consider using Drupal's theme functionality, either a tpl or a theme function for the map markup (around line 280)

The static variable is used so that when you have multiple address fields inside the same form we do not add the js for those fields twice.

#attached and drupal_add_js both should handle this situation already.

javascript control statements should still use {} (line 66). And each else, else if should be on their own new line.
See https://drupal.org/node/172169.

Ahh, that makes much more sense now. Thanks for being patient with my feedback. RTBCing.

Just a random comment about https://drupal.org/node/2127007#comment-8750019

Explicitly passing the Drupal object into the closure isn't needed, but it is an established practice. Pretty sure most people call it global import (I think Ben Cherry coined the term). With the way the JS scope chain works, the runtime will start to work its ways up the scope until it hits global, searching for the identifier. By passing it the closure, you create a binding, and since objects are passed by reference, the JS runtime will find the global object quicker. Passing in jQuery does the same thing, but with the normal Drupal pattern, the binding you create is called $. Whether it speeds up simple Drupal behaviors is debatable, but I like that style.

re: #2127007-39: [D7] Addressfield Autocomplete
Good information. I learn something new every day.

Multiple Applications

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

Project 1: https://www.drupal.org/node/2291667

Project 2: https://www.drupal.org/node/2127007

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.

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

Thew review bonus will get this finished quicker. This will likely be my next full review.

Automated Review

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

The CSS/JS look like false positives. The rest is minor.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Theere is another application for a different module, but @adammitchell closed it as a dup.

No duplication

Yes, this was a feature request in Addressfield, but it was closed as Won't Fix: https://www.drupal.org/node/1604866

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

It would be nice if the link to get Geocomplete was in the status report, and if there was a drush command to download and install it.

Add a complete path to a library file in the email, eg sites/all/libraries/geocomplete/jquery.geocomplete.js to help people double check
how to install this. It will cut down on issue when people mess it up.

Why the dependency on libraries >= 1? Why not >= 2 or no version all together?

Your JS should use the context and settings objects that are passed in via the behavior.

It looks like you have an untranslated string in addressfield_autocomplete_field_widget_info(), 'Enter one manually'. Do a pass to see if you missed others.

addressfield_autocomplete_field_widget_form() has an empty clearfix div as a prefix. Since clearfix is supposed to get rid of the need
for empty clearing divs, should that wrap it instead?

Did you test out the field in groups, etc? It looks like the code will support this, but I didn't explicitly test this.

It would be handy if this could also populate a geofield in parallel to the address.

You may want a hook_help().

Tracing out any user facing text with this is a little tough, but everything looks OK, and I did a few XSS injection tests.

Not seeing any blocking issues. The code is well written, and will be a good addition to the module space. I will be using this in the future.

Forgot the attachment..

Thanks for your contribution, adammitchell!

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.