[D7] Ubercart Ratefinder (NZ Post)

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.

There is no code in the git repository for your project. Please add your code to the repository and then set the issue status back to "Needs review".

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

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

The URL seems to be working now - I was able to click the link in #6 and see the errors. BTW the correct form of the URL is http://git.drupal.org/sandbox/southweb/2224413.git - the examples for the input textfield in your image show this form.

One thing that pareview didn't point out was that you put all your files into a uc_ratefinder subdirectory - you don't need to / shouldn't do that. Put everything at the top level of the repository.

The error messages don't mean much without knowing what commands you used to try to remove the branch. The parreview page gives you a link to the procedure to remove the master branch - if you follow those instructions it should work just fine. If it doesn't, then the instructions need to be fixed.

Drupal, like any other organization, has its own coding standards. Many of these are the result of long experience on how to avoid well-known coding problems, many are so that the module works with our automated tools (like the documentation generator), but some are just so that we share a common language and can use, maintain, and contribute to the code developed by others in this large community. Part of the instructions for apply for git permissions on drupal.org tell you how to check your code against the Drupal coding standards, and ask you to ensure your code complies before you submit your application. Once you have fixed your coding standards it will be a lot easier for the volunteer reviewers to examine your code and approve your application.

While there are a bunch of PHP beautifiers available online, I don't know of any specifically designed for Drupal code. In my opinion, because your module is small it would be quick and easy to make these changes by hand. In doing so you would learn what needs to be done, which will greatly benefit you when writing other modules or contributing to Drupal.

You still need to move the code out of the subdirectory. Take a look at some other project repositories if you're unsure what iyours should look like.

something reported by coder module about security:

uc_ratefinder.module

severity: criticalreview: security_dsmLine 157: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_dsm]
    drupal_set_message($result, 'warning', FALSE);

There's a master branch configured as default and also a 7.x-1.x branch.
The 7.x.-1.x should be set as default and the master branch should be deleted.
Follow these steps to do it properly: https://drupal.org/node/1127732
Variable names in .install file are incorrect, causing some of them still remains after uninstalling module.

Since this is a sandbox project, it can safely be deleted. I think the simplest way to sort out the mess is to do the following:

1. Delete your sandbox. (Press the Edit tab, and then the Delete button at the bottom of the screen.)
2. Set up a shiny, new sandbox. However, do not follow the instructions under "Version control" for setting up this repository for the first time. Instead:
3. Make a note of the number identifying your sandbox. In this example, I'll use 1234567.
4. In your local repo, give the following commands (remember to replace the number identifying your sandbox with the actual number):

git checkout 7.x-1.x
git remote rm origin
git remote add origin southweb@git.drupal.org:sandbox/southweb/1234567.git
git push origin 7.x-1.x

(The last two commands are the same as the last two commands in the instructions under "Version control" for setting up this repository for the first time.)

You should now have a sandbox that is correctly set up, with all your commits inside it.

Finally, update your issue summary so that the link to the sandbox and the git clone command refers to this new sandbox.

(Being a SVN veteran myself before moving to git, I know how you feel. But git grows on you!)

You can't delete the master branch, because it's set as default in your project. Go into Version Control, change to 7.x-1.x, and save. Then try deleting master.

@southweb

on your sandbox project page, go to the Version Control tab, and set your Default Branch to 7.x-1.x

You're still supposed to delete the Master branch.

@southweb, thankyou for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, http://pareview.sh/pareview/httpgitdrupalorgsandboxsouthweb2224413git reported few minor issues.

FILE: /var/www/drupal-7-pareview/pareview_temp/uc_ratefinder.module
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------
5 | ERROR | [ ] Doc comment short description must end with a full stop
122 | ERROR | [x] Whitespace found at end of line
179 | ERROR | [ ] Type hint "array" missing for $products
179 | ERROR | [ ] Type hint "array" missing for $details
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Not Sure: Does not cause module duplication and fragmentation. As I am unable to test functionality of this module so can't say whether any similar module already exist or not. As I googled not found any similar one.

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/README.md

(+) No: Follows the guidelines for in-project documentation and the README Template. Similarly project page (https://www.drupal.org/sandbox/southweb/2224413) also contains very few information that need to be extended. Also where this menu path defined in your module admin/store/settings/quotes (enable quote method here)

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

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

1. (*) uc_ratefinder_ratefinder: this is vulnerable to XSS. uc_ratefinder_api_key variable value coming form user provided input and this is passing directly to curl request, you need to sanitize it before passing further, make sure to read https://www.drupal.org/node/28984 again. As per klausi comment in #31, making correction.
2. (+) uc_ratefinder_menu: "'access arguments' => array('configure quotes')," that permission is not defined by your module in hook_permission()? Also the permission name does not really fit? As per klausi comment in #31, correcting this and moving to below list because still unable to find defined permissions in your module.

Thanks Klausi for correcting me.

Coding style & Drupal API usage

1. (*) Unable to enable this module, Is this module dependencies are correct?
   

   No release history was found for the requested project (uc_quote).     [warning]
   
   No release history was found for the requested project (uc_order).     [warning]
   
   Module uc_ratefinder cannot be enabled because it depends on the         [error]
   
   following modules which could not be found: uc_quote,uc_order

2. hook_help() is missing in your module.
3. (+) uc_ratefinder_admin_method_edit_form: what is the use of $mid = 0 param here. Also doc missing of this function. Why are you writing separate submit function as you are saving form values through varibale_set() that can be easily achieved through system_settings_form(). Also what is the use of if (preg_match('/^ratefinder_.*/', $key)) {?
4. uc_ratefinder_ratefinder(): Instead of using json_decode(), should use drupal_json_decode().
5. Add configure = admin/store/settings/quotes/methods/ratefinder in your .info file to make it directly accessible from module page.
6. Also it would be good to keep your uc_ratefinder_admin_method_edit_form() in separate admin.inc file.
7. In uc_ratefinder_ratefinder() function you are using PHP cURL to send http get request, I would recommend you to use drupal_http_request() to hit the external URL.
8. What is the need of using @ in this code if (@$rates['status'] != 'success') {, add your comment on this.
9. (+) uc_ratefinder_menu: "'access arguments' => array('configure quotes')," that permission is not defined by your module in hook_permission()? Also the permission name does not really fit?

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.

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

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

Don't sanitize variables when you pass them to cURL. It will issue a HTTP request to some other site, but it will not print anything to HTML. Therefore sanitization would be wrong here.

Also the permission question is technically not a security issue. If people use permissions that are not defined then that is not a security issue because the system will just not grant access on permissions that don't exist (only user 1 will always pass user_access()).

So I'll remove the security tag for now since those 2 things are not vulnerabilities. Feel free to add it if you find other exploits.

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.