[D7] Walkscore

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

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 sagesolutions,

Automated Review

There are still have some warnings from http://pareview.sh/pareview/httpgitdrupalorgsandboxsagesolutions2302543git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (+) I am a little confused by this line in _walkscore_my_block_view():
   $node = node_load(arg(1));
   What if this block will be on page where arg(1) is not node id? I assume, we should add conditions for this situation.
2. A better way to attach css to blocks: $block['content']['#attached']['css'][] = [a path to css]

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Automated Review

Pareview.sh passed

Manual Review

Individual user account

No: Does not follow the guidelines for individual user accounts. Seems like a company account?

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) You should check which values are available when building request. I used default Location module settings. Notice: Undefined index: province_name in walkscore_calculate_score() (line 312 of /x/sites/all/modules/walkscore/walkscore.module).

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Hi sagesolutions,

Automated Review

Nothing other than a single warning identified.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows / No: Does not follow the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) There seems to be a defect with re-geocoding. I updated the location for an address to remove the city and state and checked the re-geocode checkbox and clicked save. It kept the old latitude and longitude on the map. I created a new node and set only the street to "Swamp Rd." and it seems to have geocoded that node to the same one I had used on the previous node. I haven't tried running through it with the debugger to see if I can figure out what's going on.
2. I think many users will forget to re-check the geo-code checkbox when they edit the location. Consider adding javascript to either automatically check it when the location fields are edited or at least supply a warning in the contributor UI.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Automated Review

There are still have some warnings from http://pareview.sh/pareview/httpgitdrupalorgsandboxsagesolutions2302543git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

I haven't done any security review

Coding style & Drupal API usage

Just some recomendation:

1. in walkscore_form_node_form_alter() you can reduce code complexity if you merge the two if conditions and reverse it.
   
   

   if (!isset($settings['multiple']['walkscore']) || $settings['multiple']['max'] <= 0 || !isset($node->locations)) {
     return;
   }
   for ($i = 0; $i < count($node->locations); $i++) {
     ...
   }
         

2. You can merge conditions in walkscore_node_insert(), walkscore_node_update() and walkscore_node_delete() too.
   
   

   if (location_node_node_locations_enabled($node->type) && !empty($node->locations)) {
   

This review uses the Project Application Review Template.

I tried getting a walkscore API key, but my application is waitlisted. I'll try to come back and review again after I get that.

I have a few suggestions. These aren't strictly blocking approval, so I won't change the status.

Permission title

The permission title isn't really clear. You can expand it "Configure Walk Score" and also use a description to make it more clear what it does.

  return array(
    'configure walkscore' => array(
      'title' => t('Walk Score'),
    ),
  );

Printing the $score

A strong suggestion - this value is coming from the DB where it was stored after retrieving it from the API. There's a risk that the score will contain unexpected content (e.g. malicious strings like a cross site scripting attack). It would be better

'#markup' => $score,

Watchdog calls not translatable - could be a security risk

These lines are technically OK for security now, but they are not good for translation and could lead to an XSS vulnerability if the $statuscode or error messages from walkscore_get_error could ever contain malicious content.

    watchdog("walkscore", "Error calculating Walk Score: " . walkscore_get_error($statuscode) . " ( Error Code: " . $statuscode . " )", NULL, WATCHDOG_ERROR);
    drupal_set_message(check_plain("Error calculating Walk Score: " . walkscore_get_error($statuscode)), 'error');

Test scores a risk for XSS in the future

Similarly, these lines should be filtered (e.g. using check_plain or filter_xss) OR, if the function is guaranteed to return "safe" content then you should state that in comments so a future person modifying the code doesn't make a mistake. I like to add to the docblock something like "@return A string ready to be printed without need to filter/sanitize."

  $output .= "<div>" . walkscore_get_desc($score) . "</div>";
  $output .= "<div>" . walkscore_get_desc_detail($score) . "</div>";

Hey @sagesolutions,

I also couldn't get the API key either. It said it would take two business days to sort that out. If possible, it might be a good idea to provide a sample key for reviews as reviewers probably want to be able to do their code review in one sitting.

A few suggestions...

On line 98 in walkscore.module, I think you can just use menu_get_object() as the defaults are for node and loading arg(1). From my understanding, the node object is already loaded so you wouldn't be loading it twice.

In walkscore_form_node_type_form_alter(), it might be a good idea to set the defaults to what you are validating against. It took me a little while to figure out that I had to set the value to required instead of just adding a default value. Since the required locations to enter defaults to zero, this doesn't really affect node creation if they don't want to calculate a walk score and provides a better UX in my opinion.

On lines 119 and 125 in walkscore.module the word "description" is misspelled.

In walkscore_save(), I think you can use db_merge() to reduce your queries to just one. Reference

In _walkscore_print_test(), you might want to send the output to a theme function instead of just stringing the output together. I think this is what @greggles was mentioning above with a security risk. This also makes it more readable and uses Drupal standards.

Since I don't have an API key to test, I'm not changing the status here, but if I do receive one, I can go back and functionally test this module.

I am changing status basing on the previous comments.

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.

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.