[D7] BNM Currency rates

Multiple Applications

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

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

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

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.

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.

Update

Updated the Issue summary.

Automated Review

Git errors:
It appears you are working in the "7.x-1.0-dev" 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.

Best practice issues identified by pareview.sh

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

No: Does not follow 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

No: Does not follow 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. Your sandbox should follow the Project page template
2. You should use your template name as bnm_rates.tpl.php instead of bnm_rates_block_rates.tpl.php
3. There is no need to provide start/end comments in your bnm_rates.css
4. You should provide README.txt instead of README.md, because that's the standard Drupal way of managing the README section.
5. In your help section at admin/help/bnm_rates, instead of providing the plain text, you should provide the working URL to bnm.org in the help text.
6. Please justify, why you have created so many entries in watchdog!
7. I'm sure that above listed are not critical issues, but that need to be fixed as first priority, else your module seems good enough to me.

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.

This review uses the Project Application Review Template.

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.

Before adding tags read the issue tag guidelines. Do not use tags for adding random keywords.

To get people to review your application, you need to set its state to "Needs review". Please see project applicaton workflow.

Hi there,

I downloaded your module and here's my review:

User interface

• The Settings page shows an empty table before you first display the block anywhere. Perhaps you should pull the rates when you install the module, so that users can configure the block before they display it for the first time?
• There's no Configure link in the module list. You should add it to the .info file as described here.
• The link to the configuration page isn't listed anywhere except for the help page. In my opinion, the whole block configuration should be moved to block configuration form (the one that shows up when you click on the Edit link in the block list). You can add items to that form by using hook_block_configure(). Otherwise, you should change the path of the configuration page to nest it in the Configuration menu in the hook_menu(). I would suggest 'user-interface' (so the path should be 'admin/config/user-interface/bnm_rates').
• The configuration page has a single, uncapitalised 'settings' before the table. I suspect it has something to do with the hook_help(). You should investigate and make sure that it either says something meaningful, or does not show up at all.

Code

bnm_rates.module

• You have a switch statement with only one case in bnm_rates_block_view() on line 55. You should change it to an if.
• You could add a 'cache' key to the return array from bnm_rates_block_info(). If you specified it as DRUPAL_CACHE_GLOBAL, you would likely reduce the processing time for your block.
• You must not put variables in t(), like you do on line 59. You should rethink how you store the block subject. If it's not user configured, there's no need to store it in a variable. If it is user configured (which it isn't at the moment), you should not put it through t().
• Function bnm_rates_current_lang() is missing the docblock.

bnm_rates_functions.inc.php

• There's no need to enclose the variables included in a double-quoted string in PHP like you do on line 29. You could write that line as "Store rates for $date lang=$lang. Result: $result"
• You really need to rewrite the query on line 105 to use SelectQuery interface. As it is right now, the module does not work with SQL Server 2011. By using Drupal's database abstraction layer, you make sure that the queries will work on other SQL servers.
• Redirecting the request using drupal_goto() on line 116 is a very bad way to make sure that the block is displayed with data. You should make sure that bnm_rates_get() always returns results - you could do it by querying the database again after calling bnm_rates_pull_xmldata() on line 115, or you could make bnm_rates_pull_xmldata() return data usable by bnm_rates_get() to return the result without having to redirect.

bnm_rates_settings_form.inc.php

• You should rewrite the queries on line 87 and 95 to use Drupal's db_update() and db_select() respectively, for the reason I listed above.
• All functions in this file are missing the docblocks. You need to add them.

Other comments

• Your docblocks don't have any descriptions save for the required lines. You made a good start in some comments in bnm_rates_functions.inc.php - you should make sure that every function has a docblock, and that they all have at least a sentence or two describing what they do, for example:
  

  /**
   * Implements hook_block_info().
   *
   * Adds our block to the list of available blocks.
   *
   * @see bnm_rates_block_view().
   */
  

• I see no traces of data maintenance in your module. You poll bnm.org for rates every day and load them into the database, but when do you delete those records? Will you add a bunch of rows every day forever, without removing any?

Hi idoctor ,
Few tweaks are here fix the automated pareview issues too and the issues are listed in the given below url.

Automated Review

Fix all the Pareview issue listed in the given url http://pareview.sh/pareview/httpgitdrupalorgsandboxidoctor2438847git-1

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

No: Does not follow this module is approval for one time approval guidelines for project length

Secure code

Yes: Meets the security requirements

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. (*) nothing.
2. (+) It would be good, if you implement hook_help in your module.
3. bnm_rates.module file & bnm_rates_function.inc.php file your using the "variable_set" for geting the lang so its was a global variable you can use direclty in your function line no (bnm_rates.module:line 57 & bnm_rates_function.inc.php: line no: 49)global $language ; $lang_name = $language->language ; and get the value on it or you can define the language and get the value wherever it is needed.

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.

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.