[D8] Advance Currency Converter

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.

Automated Review

Minor suggestion from automation review :

• Git default branch is not set, see the documentation on setting a default branch.(#1659588: Setting a default branch)

Manual Review

Individual user account
[Yes: Follows ]
No duplication
No
Master Branch
No
Licensing
Yes
3rd party assets/code
Yes Follows.
README.txt/README.md
Yes Follows.
Code long/complex enough for review
Yes
Secure code
Yes
Coding style & Drupal API usage

1. Module package name is 'custom'. It should be so generic, it should be relavent to module functionality

Hi ShekharPaatni,

Thanks for contrib this module, I have found the issue which is as below

1. Whenever I am trying to enable this module using Drush then below error is appearing in my command line.

Do you really want to continue? (y/n): y
DOMDocument::loadHTML(): htmlParseEntityRef: no name in Entity, line: 5 advance_currency_converter.install:30                              [warning]
DOMDocument::loadHTML(): Unexpected end tag : div in Entity, line: 371 advance_currency_converter.install:30                               [warning]
advance_currency_converter was enabled successfully.

2. In this file "advance_currency_converter.install" you have used this function "file_get_contents" of php so I think sometimes this function gives the error at the time of fetching data, so replace this function by drupal client "\Drupal::httpClient()".

function advance_currency_converter_install() {
  // This file will call on the module installation and insert a row in tables.
  $amount = 12000;
  $from = 'BGN';
  $to = 'AED';
  // Hinting the Url to get the content of the page.
  $url = "https://www.google.com/finance/converter?a=$amount&from=$from&to=$to";
  $data = file_get_contents($url);
  $con = \Drupal::database();
  // Loading the DOMDocument object.

1. Conversion value and graph is not coming it is always show . and blank graph.
2. Needs to work on Database::getConnection() services on controller.
3. It also allows to save one currency on the configurations which by default should be two.

Hi Shekhar,

Your module seems good to me.
I found one issue:
While saving few currencies to display, it's not reflected in the frontend.

Following are some suggestions:
1. Update Readme.md
2. Add a help page
3. Add a permission page
4. Add a message for user to run cron, while selecting 'Data offline handling'
5. Give user option to select number of days to keep data, in case of offline access.

Manual Review

1) In src/Form/CurrencySettings.php
Line no 129: t() calls should be avoided in classes, use dependency injection and $this->t() instead.

2) After saving the configuration form, it redirects to /admin/config/system/currency but "page not found" error generated for first time.

3) On currency conversion block, after clicking convert button it shows the message "Converting wait" but nothing happens. No result or converted currency is available on front end.

Hi Shekhar,

Your module looks good to me and it is working on my machine, hence making it RTBC

Since this is RTBC, I'm marking fixed - I have granted @ShekharPaatni the git vetted role - you may now edit your project page to opt this project(and your future projects) into the security advisory coverage process.

User test

README and Project page - very clear about what this does, why you'd want it, and how to set it up in the beginning. Excellent.

There are some significant English syntax problems throughout the help text etc, and this needs work before tagging a stable release, but no project blockers.
The name of the project itself was probably intended to be "Advanced Currency Converter" (not "advance", which doesn't make sense). It's also unclear what makes it 'advanced' when comparing it to the earlier module 'currency_converter' that appears to offer more options.

Things seemed to work as promised. The block worked, the request happened and the AJAX refreshed. Pretty cool.
(I had some install problems on Acquia Dev Desktop today, but I debugged it to an unrelated cURL SSL certificate issue)

I was not able to test the graph today, due to more data being needed..

Meta

From a project management point of view, I would be looking for a bit more explanation of a few things that are relevant to support, accuracy, performance and security - like:
* This module calls in the third-party javascript library d3 from an external CDN.
* The currency conversion rates are provided by Google Finance using the tool at https://finance.google.com/finance/converter
* There seem to be no Terms of Use or API agreement for permissions to re-use available at https://finance.google.com/finance/converter , so we don't know how much to trust it. This can be an important legal issue for some uses.

The labelling of what you call "data offline handling" is unclear. After some experimentation I figured out that it means using a "cached" rate instead of a "realtime" lookup. That's (very) useful to be supporting.

It seems that a number of things are currently hard-coded and unconfigurable by the user at the moment. The dimensions of the graph, or the text messages shown to users. User-facing text should often be configurable by site managers. This is just an improvement suggestion at this stage so not a blocker.

Code

Seems mostly well-formed and consistent with Drupal standards. Many of the comments however are redundant or tautologous.
EG:

  /**
   * Get form id.
   *
   * {@inheritDoc}
   */
  public function getFormId() {
    return "frontpanel";
  }

  /**
   * Submit Form.
   *
   * {@inheritDoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state) {

  }

I note that in your database schema, you have chosen to store both price and date as 'varchar'

      'price' => [
        'type' => 'varchar',
        'length' => 10,
        'not null' => TRUE,
      ],
      'date' => [
        'type' => 'varchar',
        'length' => 20,
        'not null' => TRUE,
      ],

Had you considered reasons not to use FLOAT and DATETIME there?

Project application

There seem to be plenty of existing modules in this space already.
https://www.drupal.org/project/currency
https://www.drupal.org/project/currency_converter

https://www.drupal.org/project/google_currency_converter

especially appears to provide exactly the same functionality in the same way.
It's clear you would have seen these other projects when choosing a namespace for your module.
Can you describe what makes yours different or necessary, and (if there are any advantages at all) why you chose not to join forces and improve that existing one instead of competing with it?