[D8] Simple Analytics

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

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.

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.

Using Coder I found the following issues (learn more about coder and coding practices at https://www.drupal.org/project/coder

phpcs --standard=Drupal *

FILE: ...agner/Dropbox/projects/simple_analyse/lib/chart/chartist.min.css
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
1 | WARNING | File appears to be minified and cannot be processed
----------------------------------------------------------------------

FILE: ...wagner/Dropbox/projects/simple_analyse/lib/chart/chartist.min.js
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
1 | WARNING | File appears to be minified and cannot be processed
----------------------------------------------------------------------

FILE: ...rkwagner/Dropbox/projects/simple_analyse/simple_analyse.info.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
1 | WARNING | Remove "version" form the info file, it will be added
| | by drupal.org packaging automatically
----------------------------------------------------------------------

FILE: ...ner/Dropbox/projects/simple_analyse/simple_analyse.libraries.yml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
14 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...er/Dropbox/projects/simple_analyse/simple_analyse.links.menu.yml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
11 | ERROR | [x] Expected 1 newline at end of file; 3 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...r/Dropbox/projects/simple_analyse/simple_analyse.permissions.yml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
9 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 328ms; Memory: 11.5Mb

What is "intanal tracking system"? Do you mean internal?

According to https://www.drupal.org/node/2453587 and https://www.drupal.org/node/1556034 it seems nothing can be done at the moment about the minify errors so they can just be ignored.

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.

Simple Analyse is an awkward name. Simple Analysis would be correct English but you may be looking for Simple Analytics.

Hi,

I did a manual review, please see my findings below:

1. Fix issues mentioned in Automated test tool.
2. Remove commented code lines from your code, found in SimpleAnalyseConfig.php line no 28, 113, 133 and check in all files. eg:
   

   // $config = $this->config('simple_analyse.settings');.
       $config = $this->config(SimpleAnalyseService::getConfigName());

3. Use Drupal form api table class to render table instead of $html .= "<table>"; $html .= "<th>"; ... in SimpleAnalyseService.php.
4. Use hook_page_attachments() include javascript in pages. Avoid writing javascript inside template files. Found <script> tag inside simple-analyse-chart-01.html.twig, simple-analyse-pw.html.twig, simple-analyse-sa.html.twig files.
5. Found adding chartist library within module. As it is using external javascript library to create chart, it is better adding js library inside libraries folder. Then you can add the dependency of js library using hook_requirments().

Thanks,
ARUN AK

ok. I will do one more round of review. By the way did you check how Google Analytics module is adding analytics script in to pages.

HI Nuwans,

You have mentioned in libraries.yml

lib/chart/simple_analyse.css: {}

but that css or folder is not in your repository,

Correct me If I'm wrong

Downloaded and tested this module.

It would be nice if you would mention the external dependency of chartist-js on your project page. It is perfectly mentioned in your Readme file but at least for me, i don't read Readme files up until i have downloaded the module.

The module itself is working at least for Piwik. I haven't tested Google Analytics due to a lack of an Analytics account. As mentioned by others before, i dislike the name too. When scrolling through the list of modules in the issue queue, i did not expect this module to be about Analytics.

For Piwik and GA, there are existing Modules doing a great job. I would prefer using the existing Piwik module since it offers a better integration, more options and i would not really use the charts since the charts within Piwik Backend are much better in detail. But there might be a use for people that don't want to log into Piwik or GA for charts.

I have checked the site in https://pareview.sh/node/1839

Minor issues are there,

Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting

./composer.json: ASCII text, with CRLF line terminators
composer.json
Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /root/repos/pareviewsh/pareview_temp/simple_analytics.module
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
121 | ERROR | [x] Functions must not contain multiple empty lines in a
| | row; found 2 empty lines
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 286ms; Memory: 10Mb

Hi,

I have installed this module locally and i have some warning :

1. Page : admin/reports/simple_analytics/view/today

Notice: Undefined property: stdClass::$count in Drupal\simple_analytics\Controller\SimpleAnalytics->viewToday() (line 65 of modules/simple_analytics/src/Controller/SimpleAnalytics.php).

Drupal\simple_analytics\Controller\SimpleAnalytics->viewToday()

2. Page : admin/reports/simple_analytics/view/today

Notice: Undefined index: a2327acfc46df0f7dd1891488361f49b in Drupal\simple_analytics\Controller\SimpleAnalytics->viewToday() (line 97 of modules/simple_analytics/src/Controller/SimpleAnalytics.php).

Drupal\simple_analytics\Controller\SimpleAnalytics->viewToday()

3. Page : admin/reports/simple_analytics/view/today
Notice: Undefined property: stdClass::$visit_last in Drupal\simple_analytics\Controller\SimpleAnalytics->viewToday() (line 95 of modules/simple_analytics/src/Controller/SimpleAnalytics.php).

4. Page : admin/reports/simple_analytics/view/history
I just installed this module and i have this page : http://i.imgur.com/SuoUM81.png . I found it a bit strange, maybe it's better to only show the date since the module was installed and the statistics start to be saved

Hello, @NuWans,

I've made a manual review of your code and created few issues under module issues queue - https://www.drupal.org/project/issues/simple_analytics.

Please take a look, thanks!

Automated Review

Pareview details: https://pareview.sh/pareview/https-git.drupal.org-project-simple_analyti...

Review of the 8.x-2.x branch (commit 3c1496d):

• Your README.txt does not follow best practices (headings need to be uppercase). See https://www.drupal.org/node/2181737 .
  • The INTRODUCTION section is missing.
  • The REQUIREMENTS section is missing.
  • The INSTALLATION section is missing.
  • The CONFIGURATION section is missing.
• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
  

  ./lib/live.js:                                                        ASCII text, with CRLF line terminators
  ./lib/simple_analytics_chart.js:                                      ASCII text, with CRLF line terminators
  

• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script.

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

No: Does not follow the guidelines for in-project documentation and/or the README Template. See pareview

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: List of security issues identified.

Coding style & Drupal API usage

1. (*) sanitize $_POST input in SimpleAnalyticsTracker.php and SimpleAnalyticsService.php
2. (*) Bad line endings were found, always use unix style/LF terminators in JS files.
3. (+) Visitor's Setings should be Visitor's settings in simple_analytics.links.task.yml
4. (*) Log notices: Notice: Undefined property: stdClass::$extra in Drupal\simple_analytics\Controller\SimpleAnalytics->viewToday() (line 77
5. (*) Log notices: Notice: Undefined property: stdClass::$LINK in Drupal\simple_analytics\Controller\SimpleAnalytics->viewToday() (line 77
6. (*) Log notices: Notice: Undefined property: stdClass::$Duration in Drupal\simple_analytics\Controller\SimpleAnalytics->viewVisiter() (line 198

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.

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

I am closing this application due to lack of activity.