[D7] - Most Viewed per Content Type

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

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.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2132599

Project 2: https://drupal.org/node/2020647

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Sorry for the noise, the robot stumbled a bit on broken issue queue search links. Your other application is closed already, so everything fine here.

Couple things.
Your git repo link should be:
git clone http://git.drupal.org/sandbox/marcoscano/2132379.git mvct

The link you have has your username in it which will fail for anyone trying to review the project.

The review bot is very picky and it's looking for doxygen style comments. What it's seeing is this. This is your function doc block

/*
 * Helper function to get the existent content types from the database.
 *   returns an array in the form ("content type machine name" => "human name").
 *
 * @return Array
 *   Returns an array containing all active content types from the database.
 */

That should be

/**
 * Helper function to get the existent content types from the database.
 *   returns an array in the form ("content type machine name" => "human name").
 *
 * @return Array
 *   Returns an array containing all active content types from the database.
 */

Notice the single extra "*" at the first line of the comment blog. Some of your doc blocks are right some aren't. That should clear up the auto review issues found.

Forgot to change the tagging...

Woohoo... All clean! I'll test the module out tomorrow if no one else does before me.

Hi,

1. In your mvct_settings form, you have called your custom function for content types. Instead of this you can use node_type_get_names() directly in #options.
2. You should add your mvct.admin.inc and mvct.views.inc files in info file.
3. You have defined your helper functions in module file. Instead of this, define them in your mvct.admin.inc file as module file parses on every page request and will make it heavy.

Thanks

My second review of this module. Module works as described and provides statistics on contents types viewed.

Developer is correct in his statement. His function only returns active content types. The core function returns all content types despite their condition.

Other changes were made according to feedback.

If you want only active content types, then try this https://api.drupal.org/api/drupal/modules!node!node.module/function/node_type_get_types/7

Stil if you want to access data directly from the db, then use caching so that once get the data, it available in the cache and no need to access data on every page request.

thanks

No problem! Glad to help.

Marcoscano is correct. The function in #13 is calling this function: https://api.drupal.org/api/drupal/modules%21node%21node.module/function/...

That will return all node types available in the database no matter what their status is. For the purpose of showing stats to your user, you don't want to show them stats on a node type that you may have disabled for some reason.

• Do reviews on other projects to get: PAReview: review bonus.
• Don't change the status to mayor or critical. There is a lot of us and few of them.
• You should make necessary indexes for the db schema.

Ok, thanks didn't know that.
You need indexes if you do queries where you have conditions other than the primary key.

Hi marcoscano,

You should use drupal inbuild function node_type_get_types() to fetch all active content types from the database in place of mvct_get_content_types().

Hi marcoscano,

I am agree with you but i review your query. you are using only two fields(type and name) but you are fetching all fields from node_type table. So it is needed optimization in query.

Second thing: Suppose no content type is active in that case your form will display the
Title: Select the content types that will be monitored and
Button : Save Configuration.
Actually there is noting to save and when i click on the button it will dispaly a message "The configuration options have been saved." Actually there is noting to be saved.

Hi marcoscano,

Pareview automated testing found a number of issues:

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /var/www/drupal-7-pareview/pareview_temp/mvct.admin.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
28 | ERROR | else must start on a new line
70 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mvct.info
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
6 | ERROR | Files must end in a single new line character
6 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

Codespell has found some spelling errors in your code.

./README.txt:6: usefull ==> useful

I found some issues when conducting a manual review:

1. Double space in @file comment in mvct.views.inc (sorry!)
2. Double space in @file comment in mvct.install (sorry again!)
3. In mvct.info you're including mvct.admin.inc, you don't need / shouldn't do this here. You're also including it in your menu hook which is correct. By including it in the info file you're including it on every page of your site.
4. Double space in @file comment in mvct.admin.inc (sorry again! copy paste error? :) )
5. In mvct.admin.inc in mvct_get_content_types(), you should use db_query() instead of db_select() for simple queries where you're not conditionally adding conditions etc. See: https://drupal.org/node/310075
6. mvct_get_record() query as above.

Once the above minor issues are sorted think you're good to go.

Hi,

The menu item 'admin/config/mvct' in file mvct.module line 14, is not reflected under any section in the configurations page.
It would be much better if users can see the link to your module settings page.
You can probably make two menu items so that it appears in the configuration page or can be included inside any other menu item already present in the configurations page.

Hi marcoscano,

Pareview automated testing found a number of issues:

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
20 | WARNING | Line exceeds 80 characters; contains 81 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mvct.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/mvct.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
16 | ERROR | Do not use t() in hook_menu()
--------------------------------------------------------------------------------

I will take a look through the code in some more detail and report back.

Good work

Hey Marcoscano,

I've tested your module, and must conclude that it works fine.
However, there is one small change I'd implement in code. If I were you, I'd use the Database Abstraction layer instead of direct calls to db_query().
I've included a patch with the SQL statements rewritten to DBTNG. On the plus side, it saves you one loop as well :-)

I have one minor optimization point to make:

On line number 43 and and 48 you have the same assignment statement being used.

$record->last_viewed = REQUEST_TIME;

You can remove the former one.

I was going to build a module specifically to track views of my blog content type only and then came across this. I have tested the module and it works as described. I have also reviewed the code and do not see any problems with its implementation.

Non-blocking issues:

• You might want to note somewhere that this module only counts the 'full' view mode. With modules like display suite that use all kinds of other view modes, I think the clarity would help.
• Providing a default view that makes use of your data would be a nice addition

Checked for security, licensing, Drupal API, and individual account issues.

Thanks for your contribution, marcoscano!

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 reviewer(s) as well.