[D7] Open Exchange Rate Money.js

Hello,

When activating the module I get the following php notifications:

Notice: Use of undefined constant MONEYJS_PAGES_NOTLISTED - assumed 'MONEYJS_PAGES_NOTLISTED' in include_once() (line 9 of moneyjs.module).
Notice: Use of undefined constant MONEYJS_PAGES_LISTED - assumed 'MONEYJS_PAGES_LISTED' in include_once() (line 10 of moneyjs.module).
Notice: Use of undefined constant MONEYJS_PHP - assumed 'MONEYJS_PHP' in include_once() (line 11 of moneyjs.module).
Notice: Use of undefined constant MONEYJS_PAGES_NOTLISTED - assumed 'MONEYJS_PAGES_NOTLISTED' in include_once() (line 9 of moneyjs.module).
Notice: Use of undefined constant MONEYJS_PAGES_LISTED - assumed 'MONEYJS_PAGES_LISTED' in include_once() (line 10 of moneyjs.module).
Notice: Use of undefined constant MONEYJS_PHP - assumed 'MONEYJS_PHP' in include_once() (line 11 of moneyjs.module).
Notice: Undefined variable: output in moneyjs_help() (line 20 ofmoneyjs.module).

I believe your constants should be like define("MONEYJS_PAGES_NOTLISTED", 0); with the first argument a string(see define).

You may find drupal_http_request useful as an alternative to curl or possibly as a fallback if curl is not installed.

I installed the module and successfully got data after running a cron. One thing I was confused about was whether drupal commerce was a dependency or not, your readme seemed to indicate it was required? If this requires drupal commerce it should be indicated in your .info

This is not fixed? See https://drupal.org/node/532400

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

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

Hello vwX, your project cannot be cloned using the Git command mentioned in your post.

Added information about updates made.

Hello vwX,

I reviewed your module. Here is my report to help you to improve it:

• Where is the accounting.js file you are talking about in your project page/README.txt? Please give download links too, and, even better, provide them in the online help of your module.
• You hard-coded the URL of the "cron job" link on the moneyjs module configuration page. This is not working on my system with clean URLs turned off. You should use l() or url() to generate links.
• Data was not populated as expected after following this URL (manually, since the link was not working). I also got the following warning:
  Warning: call_user_func_array() expects parameter 1 to be a valid callback, function '_moneyjs_cron_run' not found or invalid function name in menu_execute_active_handler() (line 517 of /XXX/includes/menu.inc)..
  However, running cron from the default administration location allowed your module to work and populate the data correctly.
• I copy-pasted the sample code from your project page/README.txt in a node. The form is rendered, but the dropdown list of currency is empty. Maybe this is due to the fact that I'm lacking the file accounting.js?
• You might want to improve the quality of field descriptions. For example, If you are a subscriber hecking this box... should be If you are a subscriber, checking this box..., api should be spelled API, etc...

I would be glad to review it again once you went into these issues.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

Review of the 7.x-1.x branch:

FILE: /home/klausi/pareview_temp/moneyjs.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 55 | WARNING | Unused variable $moneyjs_api_key.
 60 | WARNING | Unused variable $jslibs.
--------------------------------------------------------------------------------

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. moneyjs.js: all user facing text must run through Dupal.t() for translation.
2. moneyjs_menu(): 'file' => 'moneyjs.admin.inc', for the second menu item is missing?
3. moneyjs_form(): HTML tags should be outside of t() where possible.
4. variable_set('moneyjs_fx_rates', json_encode($exchange_rates->rates));: why do you JSON encode the variable? It will get serailized in the DB anyway, so this is not necessary.
5. _moneyjs_load_library(): shouldn't you require the libraries module in order for your module to work at all?

Hello vwX, thanks for the fixes.

I reviewed your module again, here is my report:

• accounting.js is now easy to find.
• Run cron link is now working.
• Data is now populated as expected when following this link.
• The sample code seems to work better (the amount is not 999.99), however the drop-down list of currencies is still empty, I guess it is because I did not check Open Exchange Rate subscriber, right? If yes, maybe you should remove the drop-down list when empty, and mention what are the source currency and the target to make it more obvious.
• Descriptions are better, but still references to api in lowercase. By the way, you should be consistent with https://openexchangerates.org/ and use the term App ID instead of API key to not confuse people.

And there's a new issue:
At first, I tried without the libraries module enabled (I placed the 2 javascript files in the folder of your module, as indicated), this did not work. I had to put the javascript files in libraries/moneyjs and turn on the libraries module to have it working.
For simplicity and reusability, maybe you should only support the libraries method, and add the libraries module as a dependency?

I'm not sure if these reports are critical enough to change the status of the review, so I leave the decision to other reviewers.

My call on libraries is that though "Libraries API module" is nice, and preferred, it's not always a hard dependency if the install instructions are clear enough and internal error handling is sufficient.
If placing the files manually is enough and the module detects and validates it (eg hook_requirements()), it's not always necessary to bring in external dependencies.

My review:

In the modules page:

money.js intergration
Drupal plugin for money.js

Typo in the name, and the description is undescriptive. I suggest "Provides currency conversion tools"

While installing.
As above, if you are not using libraries API module, you should check for dependencies yourself and use hook_requirements so it registers on admin/reports/status

When visiting "Admin : Configuration" I would expect the configs for this tool to be placed under "Web services", it's not "Development".

There again, the title and description

Moneyjs
Configuration for moneyjs module

The description Is mostly just a useless tautology. Use that text to use OTHER keywords that might explain to the user what they may find there.

The install process was OK. I'd like to see a watchdog message about the job that was done in the cron run, to help identify problems, as I'm used to encountering problems with behind-the-scenes network connections (many of our sites are firewalled and mysterious network failures can be hard to trace if not logged).

Although I see that this is just an API base, I think it could be helpful to provide that example block you describe as a quickstart.
I tried following the instructions exactly, but I think you actually need to have an 'unfiltered' text format available and use it.

Once installed, I saw a bunch of extra code insterted into my Drupal.settings, but javascript money.js was throwing "Uncaught ReferenceError: accounting is not defined "

I tried having the downloaded libraries in the module folder, and the block code as given, but it still didn't seem to be working. Your check for _moneyjs_load_library() doesn't seem to be working in the non-libraries-api mode.

Review of the 7.x-1.x branch:

• ./README.txt: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

• "How much is " + price + " in your currency?": all user facing text must run through Drupal.t() for translation.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to sreynen as he might have time to take a final look at this.

As openexchangerates.org accounts are rate-limited, I'd suggest allowing users to rate-limit the requests. You could grab code for that from https://drupal.org/sandbox/sreynen/1993994 But that's not at all a blocker, so...

Thanks for your contribution, vwX!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewers as well.