[D7] BNR Exchange

Hi epoint,

You need to add a description to this issue. You can use the same as your project page.

The included link to git repository is protected by crendentials (your Drupal page user account). It's recomended to create a 7.x-1.x branch in your git project and delete the master branch. You can use the following links:

• http://drupalcode.org/sandbox/epoint/2185357.git
• git clone --branch master http://git.drupal.org/sandbox/epoint/2185357.git bnr_exchange: With master branch
• git clone --branch 7.x-1.x http://git.drupal.org/sandbox/epoint/2185357.git bnr_exchange: When you create the 7.x-1.x branch

Also, there is a lot of errors in your modules code. You can use PAreview to test it. Look your results here

Cheers!

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.

Hello!

Here comes a manual review if the code.

- what's up with "BNR" ? Unless you plan to release a set of "BNR" modules, consider not using a "package" in the .info file.

- schema definition, requirements, etc in the .install file are nicely done. But I think you could use hook_enable() instead of hook_install() to refresh data.
- also, you have a file_get_contents() call on a remote URL which requires some fopen configuration in php.ini. Consider adding it as a requirement in hook_requirements().

I don't see why we need a separate module for every source of currency information. There's at least one other pending application that does the same thing but with a different bank: https://drupal.org/node/2238127

Rather than writing your own module with only a few features, why don't you write an add-on for the Currency module (https://drupal.org/project/currency)? Currency defines a plugin architecture so that you can write your own plugin to get currency information from your preferred source, in this case the National Bank of Romania. If you do this, you will get all the features of the Currency module for free, plus there will be a much larger community of users/developers to maintain and improve the code for you.

Hi Epoint,

Here is the list of Errors which are to be corrected -
http://pareview.sh/pareview/httpgitdrupalorgsandboxepoint2185357git.
Resolve these errors.

Manual review of your module.
1. Line 6 of README.txt file says - "the way they show in the bock view". Please correct the spelling of BLOCK.
2. File bnr_exchange.install has some errors which are to corrected.
a. Line 10 has

   function bnr_exchange_install() {
   }

which is a empty installation hook and empty installation hooks are not required.
b. Line 61 has
elseif(!ini_get('allow_url_fopen')) {
which is not correct drupal coding standard. 1 Space needed after elseif () {
c. Line 66 needs 6 spaces before array closing indentation braces instead of 8 spaces.

Module works in terms of functionality. Block configuration works as expected.
I would like to request you to add one extra feature that will increase the usability of the module drastically is ->
We get to select two currencies and display exchange currency rate among them instead of hard coding it to RON,thus making this second currency as configurable.

Hi Neetu,

We have corrected the errors and testing with pareview shows no errors now. About your request to add the new feature with configurable second currency, I'm afraid it won't work. As the name of the module suggests, it shows official exchange rates from BNR. We get the values from National Bank of Romania and in the xml they only provide exchange rates of most used currencies among RON.

Thanks!

Manual Review

Individual user account

NO: the user account is the name and the logo of a company based on Romania. take a look to http:// epoint.ro/team

No duplication

Yes: I think so, but it's my oppinion.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: I think it follows the licensing requirements

3rd party code

Yes: I didn't found any 3rd party code so it's ok.

README.txt

No: the README.txt didn't mach the template and (IMHO) it's not clear.

Code long/complex enough for review

Yes: 1429 lines and a bunch of funtions.

Secure code

Yes. : I checked al SQL querys and all of then use de database API. Didn't found any cross site scripting.

The module works and do exactly what the authors says that must do.

Thanks for our contribution.

I tried saving some of the currencies on setting page.

URL: admin/config/bnr_exchange/settings

Got confirmation Message on the top but check-boxes are not selected by default.

Sorry, I cannot reproduce the bug. Maybe you clicked Update instead of Save?

As in #2186013-14: [D7] BNR Exchange, I modifed the README.txt to follow standards and added INSTALL.txt. But I don't know how I can change the owner of the project.

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.