[D7] Taxonomy Tracking

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

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.

Git clone command for the sandbox is missing in the issue summary, please add it.

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

Your README.txt file says that civicrm module is required, but it's not
required in info file of module.

Also PHP_CodeSniffer found a lot of issues in your main .module file:

EDIT: removed long pareview.sh dump.

Small code overview.

• Read what is written under issue tags form field!
• Fix pareview issues.
• Remove master branch and make the other branch default branch: https://drupal.org/empty-git-master
• Write something useful to your commit messages!
• Use database abstraction layer instead of regular queries, that will make the use of your module possible on other databases than mysql. For your reference: http://browse-tutorials.com/tutorial/sql-queries-examples-drupal-7

You have some require_once() calls to relative paths, like require_once 'CRM/Core/BAO/...'
Where is the CRM/Core/... located? If is provided by other module, why do you use relative paths using your module folder as root?

Before adding tags read the issue tag guidelines. Do NOT use tags for adding random keywords or duplicating any other fields. Separate terms with a comma, not a space.

I think you must have misunderstood how the review systems works.

Please note that you're not supposed to change the status to "Reviewed & tested by the community" (RTBC) yourself. Doing so will just place your application in limbo until status is set back to "Needs work". To speed things up, I've done that for you.

Also, do not set priority to "Major" without stating explicitly why your case is not "Normal".

After you think you've fixed all the problems pointed out in all past reviews, you change the status from "Needs work" to "Needs review". This indicates that you think the project is ready for a new review. You're not supposed to review your own project.

After the reviewer has had time to review the new version, he or she has two main options: To point out that there are still points that need to fixed, and set the status to "Needs work". Or to set the status to "Reviewed & tested by the community" to indicate that the reviewer now thinks the project is ready to be promoted into a full project. Note that this status in no guarantee of promotion. There may be other reviewers that disagree, and that may push it back down to "Needs work".

Also: The automated review of your project by PAReview has found some issues with your code. As coding standards make sure projects are coded in a consistent style we ask you to please have a look at the report and try to fix them.

@vanyamtv, there has been no manual review of your application that says it is ready to be approved yet.

Just now there is around 83 projects in the queue with status "Needs review". The oldest is more than six weeks old. We usually go by "last update" date when we prioritize, so updating the issue by asking to be promoted has the unfortunate side-effect that the "last update" date is updated, pushing you to the very end of the queue again. Please just be patient.

There are very few people doing reviews, and they're all volunteers working for free. To give your application a higher priority, please join the review bonus system. If you participate in the system by doing manual reviews, you can put yourself on the high priority list. Applications with PAReview: bonus tag are more likely to receive attention from reviewers and git administrators.

Your project has the added disadvantage of having a very poor project page. When I visit your project page, there is absolutely nothing there that tells me why I should be interested in your module or why I should review it. So - given that there are 83 modules that is looking for a reviewer - I pick another where at least the use case (and whether it suits me or not), is made clear on the project page. Please take a moment to make your project page follow the Project page template and it may be a good idea to also read tips for a great project page.

@vanyamtv, first of all thanks for your patience and contribution.

As @gisle suggested you, get a review bonus tag by joining review bonus system.

Also your project page contains very less information but README.txt file contains much information. I think your project page should contains this all information in better way that are mentioned in README.txt file.

Manual Review:
1. Your admin configuration form should be in admin.inc file.
2. There is no need to use extra variable ($tracking_access) in taxonomy_tracking_menu() function.
3. Better to use REQUEST_TIME instead of time().
4. In your README.txt file following line is written but this is not clear how CiviCRM will use here.

Admin can select vocabularies terms of which will be tracked in the system.
   After selecting vocabularies and saving settings corresponding Custom group
   will be created in the CiviCRM (CiviCRM > Administer > Customize Data and
   Screens > Custom Fields, civicrm/admin/custom/group) and module starts to
   collect tracking data for selected vocabularies. 

Also https://www.drupal.org/project/civicrm don't have any recommended release. My concern is here, how this module will work with civiCRM and without civiCRM. Because from current documentation it is not clear whether civiCRM is really useful to use or not.

Might be I am not getting complete understanding as I have never used civiCRM, so get update your project page with some more detailed information, it would give better understanding to review your module.

In most of your module function you have used following code, it indicate if civicrm is not there, then just return. I mean if its optional then most of your code is unused that looks wired.

if (!function_exists('civicrm_initialize') || !civicrm_initialize()) {
    return;
  }

PS: First of all you please update your project page and describe in more detail what exactly civicrm is doing in your module.

Thanks again for your hard work!

@gisle or any of the regular project reviewers. When you come across a CiviCRM related module that needs review, please ping me. The advice these reviewers are giving @vanyamtv is making the module worse.

CiviCRM is not a simple module hosted on Drupal.org. It is a CRM with a codebase larger than Drupal designed to be paired with Drupal, Joomla, or WordPress as a CMS. It includes Drupal modules and support for Views and Rules, but when developing custom functionality the majority of integration is done by leveraging the API. When you donate or join the Drupal Association, you are doing it using CiviCRM. When you see a URL like https://assoc.drupal.org/civicrm/contribute/transact?reset=1&id=83, Drupal is really acting as a dumb menu path manager that wraps the CiviCRM output in a Drupal theme.

Because CiviCRM is not a strict dependency of the module, if (!function_exists('civicrm_initialize') || !civicrm_initialize()) { is a legitimate way to first check if the CiviCRM module is enabled and then check to see if it initializes successfully.

You don't need $GLOBALS['civicrm_root'] or require_once 'api/api.php'; Once CiviCRM is initialized CRM/Core/BAO/CustomGroup.php will be found. Look at http://cgit.drupalcode.org/civicrm_cron/tree/civicrm_cron.module#n141 for an example.

Same for api/api.php, once CiviCRM is initialized you can just use the API. Look at http://cgit.drupalcode.org/civicrm_certify/tree/civicrm.api.inc for an example.

I haven't installed this module yet, but if someone wants to test it before I get a chance they can spin up and instance of Drupal and CiviCRM on Pantheon using https://dashboard.getpantheon.com/products/civicrm_starterkit/spinup

Looking at the code, it seems like everything in your hook_cron is only required if CiviCRM is enabled. Why not check for CiviCRM at that point and then assume it is there in the functions that follow?

Would it make sense to split the functionality into 2 modules? The main taxonomy_tracking module to handle the Drupal side of the functionality and a new taxonomy_tracking_civicrm sub module that has taxonomy_tracking and civicrm as dependencies that handles the cron functionality?

A few other notes on your PHP...

When you call taxonomy_tracking_civicrm_get_user_activity_group, you are always passing the result of variable_get('taxonomy_tracking_user_activity_level_group_id', NULL). Why pass anything. Why not just use variable_get('taxonomy_tracking_user_activity_level_group_id', NULL) in the function?

http://cgit.drupalcode.org/sandbox-vanyamtv-1909610/tree/taxonomy_tracki...

if ($view_mode == 'full' && $GLOBALS['user']->uid > 0) {

could simplify that to...

if ($view_mode == 'full' && $user->uid) {

I downloaded the module and it looks good at first glance. I believe heavily in grammar and language for any module, so your configuration screen at admin/config/taxonomy-tracking needs some fixes.

1. VOCABULARIES SETTINGS should be VOCABULARY SETTINGS. The heading should be singular.

2. "After saving settings corresponding Custom group will be created in the CiviCRM."

• About the second bullet, some people, especially drupal newbies, do not know what the CiviCRM is, so include a link to it or add markup in your configuration form to explain what that is.
• Is "Custom group" really capitalized? Just a question.

Some issues with the README.TXT file

The Configuration screen of the Readme.txt file does not seem to match the Configuration screen at admin/config/taxonomy-tracking.

The README.TXT file has 3 steps (as listed below), but the second and third items are not present on that screen, so it is a little confusing.

  - Define possible activity levels for user which will be used for calculating how often user visits tracked pages on the site;
  - Determine a frequency of data aggregation
    (there are 2 possible values available: every cronjob running or
     every 24 hours).

Possible Solution: Can the file be rewritten to tell people where to go to accomplish the last 2 steps?

Hi vanyamtv,
Here are my reviews and comments about your module:

Automated Review

Reviewed the code. Main issue I see is many lines in your code exceed the 80 character limit per https://www.drupal.org/coding-standards#linelength.

Code lines are not required to be under 80 characters, anything else that you found or should this be RTBC instead?

Other than the code lines, I saw nothing. Please clarify why you say code lines are not required to be 80 characters. The documentation clearly states

Line length and wrapping
The following rules apply to code. See Doxygen and comment formatting conventions for rules pertaining to comments.

In general, all lines of code should not be longer than 80 chars.

There are many exceptions listed on https://www.drupal.org/coding-standards#linelength , so this is a general rule that can be broken many times (it even has an ambiguous "etc" there, so you can invent your own exceptions). And on its own it is surely not an application blocker that would warrant to put an application to "needs work".

Thanks for the feedback. I'll keep that in mind next time.

This project application is not fixed? See the workflow: https://www.drupal.org/node/532400

The current "wait time" for a new developer who wants to contribute a module and doesn't review 3 other projects for the review "bonus" is 42 weeks.

https://www.drupal.org/project/issues/projectapplications?status=14&page=1

I'm not sure why this is called a bonus. With the current backlog and pace of the project review admin group is able to do a final review of project, if you don't review 3 other projects it is increasingly unlikely your project will ever be promoted to a full project.

this application is not fixed since it still needs review? See the workflow: https://www.drupal.org/node/532400

@vanyamtv PLEASE stop changing this issue to Fixed. The issue will only be fixed once your project has been promoted. Only a Project Review/Git Admin can do that.

Not following the instructions is only annoying the people you need to volunteer their time to help.

From https://www.drupal.org/node/1975228

1. Do at least 3 manual reviews of separate project applications in the main issue queue. While not mandatory, there is a template that you can use to make sure you cover all of the necessary points in your review.

While you added the reviews, you didn't use the template found at https://groups.drupal.org/node/427683. That is the same template used by @veso_83 to review your project in https://www.drupal.org/node/2142003#comment-9288421. Full reviews of both what you found to be correct and the issues have much more value.

2. Reference those 3 reviews in the issue summary of your own application issue. You can do this by editing the first post (the issue summary) of your project application issue. Then create a new section called "Manual reviews of other projects" and add the links to the exact review comment in the reviews you have done, e.g. like this one http://drupal.org/node/1381726#comment-5418942

Again, you only added a link to the node. Not the comment with your review in each of those. There are far more reviews that need to be done than people qualified and willing to do them. You need to follow the instructions so your project can be reviewed as quickly as possible.

3. Then add the following tag to your application: PAReview: review bonus using the 'Issue Tags' field in the comment form for your application issue.

Please redo your reviews using the template and update the links in this issue to point directly to each review. Only after doing that should you add the "PAReview: review bonus" tag to this node.

Finally I would start a new discussion in http://groups.drupal.org/civicrm about the fact that you are looking for more people to review your work. Most of the developers who work in Drupal and CiviCRM are subscribed to that group so we get an email any time something new is posted.

Project page: what is the purpose of this module? what use case does it solve? What are the differences to other existing project? Please update the project page according to https://www.drupal.org/node/997024

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.