[D7] AddressField CR

Nice work.
Pareview clean, excellent code comments too. I found some grammatical errors in the code comments though.

IMO, address_cr_addresses function is too large, and including it in the .module makes the installed site a bit more heavy.
Consider moving the core logic to a different file, and to include the file only when the function was called.
I'm not changing the status to Needs Work because I didn't find any blocker.

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.

Hi,

with this info:

GIT: git clone --branch 7.x-1.x kevin.coto@git.drupal.org:sandbox/kevin.coto/2408001.git addressfield_cr

Your project cannot be cloned by Git from any reviewer. Please provide a command without any dependency to your user account in your project application issue description: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/kevin.coto/2408001.git addressfield_cr

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/kevin.coto/2408001.git addressfield_cr
Above works just fine.

Place this in your project application as central position. "GIT: git clone --branch 7.x-1.x kevin.coto@git.drupal.org:sandbox/kevin.coto/2408001.git addressfield_cr" is linked to the kevin.coto account.

+fixed git clone cmd

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.

Git clone command git clone --branch 7.x-1.x http://git.drupal.org/sandbox/kevin.coto/2408001.git addressfield_cr works fine.

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

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

Fixed code standards errors.

Hi alarez,

Very cool add-on for addressfield! I could see this greatly reducing errors for sites with lots of manual entry.

I needed to use https for your git clone to work: git clone --branch 7.x-1.x https://git.drupal.org/project/addressfield_cr.git addressfield_cr

Install was clean and drush found dependencies. Module worked as described, providing additional district options to addressfield for Costa Rica.

Pareview (http://pareview.sh/pareview/httpgitdrupalorgprojectaddressfieldcrgit) has a few suggestions, namely:

• make sure functions you write are prefixed with your module name to avoid naming conflicts (get_suffix_id)
• make sure lines in your README.txt wrap at 80 characters if possible
• correct two minor spelling errors in README.txt (wich ==> which, arround ==> around)

Manual review:

• Can you name the keys of the option array somethings that is more human readable than 1, 2, etc, such as 'district' or 'zip'?
• It looks like you may need to add check_plain (see https://www.drupal.org/writing-secure-code) on your $address array for user inputs

Thank you @jimafisk for the git clone link. The one in the issue summary is not working.

I don't understand why this is in this issue queue if the project is already a full project... But here's my review anyway:

Functionality and user interface

I installed the module successfully and the dependencies are set up correctly. I would think a bit more about a few more things:

• I would rename the 'Costa Rican Address form' checkbox to something like 'Interactive Costa Rican form' to indicate better what users can expect to happen on the form.
• The 'Enter Address' field is printed in default output of the address. When you print the address, I'm pretty sure that you don't need to provide the information about how the address was produced. In my opinion you should not output that field.
• The individual fields of the address are output inside span elements without any spaces in between. This makes the address look like this if you don't style it: 'By DistrictSan José, EscazúSan Rafael10203225'. You should make sure that the address comes out like it should by default without any additional styling. I would put separate lines in div elements, and the fields that should be output in one line in spans separated by spaces:
  

    <!-- This is current output: -->
    <div class="addressfield-container-inline locality-block country-CR">
      <span class="name_line">By District</span>
      <span>San José</span>, 
      <span class="locality">Escazú</span>
      <span class="district">San Rafael</span>
      <span class="thoroughfare">10203</span>
      <span class="other">225</span>
    </div>
    <!-- You should output the same markup as the default Costa Rican address without your plugin: -->
    <div class="field-item even"><div class="street-block">
      <div class="thoroughfare">Belén de Nosarita</div>
      <div class="premise">Yellow house</div></div>
      <div class="addressfield-container-inline locality-block country-CR">
        <span class="postal-code">50207</span> <span class="locality">Nicoya</span></div>
      <span class="country">Costa Rica</span>
    </div>
    

• I found a bug: when you edit an existing field with values in it and try to change the 'Enter Address' option, the form does not change to reflect that. It only happens on my Windows environment (with MS SQL Server 2011 and IIS 10) and does not on Linux.
• Another bug: When you change the 'Enter address' from 'By District' to 'By ZIP Code', the Province, Canto, and District are not cleared. You should either fill in the ZIP code to what it was before user changed the 'Enter address' field, or clear all fields.

Code

• As @jimafisk mentioned, you should use named constants or strings as the $option parameter in address_cr_addresses(), or at the very least you should list the accepted values in the doc block.
• I don't think that an array with all data declared in a function is the best way to store the address information. This amount of data definitely belongs in the database. Selecting the right data will then become a question of a well crafted select query.
• name_line is a bad name for the field that means autofill type.
• Values of options of the name_line field are meaningless ('1', '2'). You should either use strings or named constants.
• locality and province fields are both given canton class. Is that right?
• As specified in Drupal documentation for t(), 'the first argument should always be in English'. You should either not use t() in Canton and Province #empty_option option, or write them in English.
• You should add a module-specific prefix to the function name get_suffix_id. This name is so generic that any module could try to define it. I prefix all functions that are not hooks with _module_name.

I hope my comments help!

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.