Recently read module displays a history of recently read nodes by currently logged in user. Site administrators can enable blocks containing recently read links for each content type separetly.

This may be useful i.e. for displaying a history of recently viewed product on the e-commerce site.

This module is a response to http://drupal.org/node/65679

As far as I know, there is no other module that provides the similar functionality.

Comments

venutip’s picture

venutip commented

Have you uploaded this module to your git sandbox yet? If so, please provide a link.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Postponed (maintainer needs more info)
pgorecki’s picture

pgorecki commented

Sorry, I forgot the most important part:
http://drupal.org/sandbox/pgorecki/1080970

venutip’s picture

venutip commented
Status: Postponed (maintainer needs more info) » Needs work

- Found one security issue (line 136) and several coding standards issues. Run your module through Coder on the most critical settings and fix the problems it reports:

http://drupal.org/project/coder

- Lines 191, 76: use curly braces. From http://drupal.org/coding-standards#controlstruct

You are strongly encouraged to always use curly braces even in situations where they are technically optional. Having them increases readability and decreases the likelihood of logic errors being introduced when new lines are added.

- Use of hook_exit() makes this module incompatible with aggressive caching. Not a problem per se, but you might want to see if you can achieve the same results using hook_init() instead.

pgorecki’s picture

pgorecki commented
Status: Needs work » Needs review

Fixed the security issue, fixed problems reported by Coder.

meba’s picture

meba commented
Status: Needs review » Needs work 
        $blocks[$key]['info'] = t('Recently read - !type', array('!type' => $types[$key]->name));

Why is ! here? Not needed and possible security issue (although I doubt Content type name might contain XSS. Good practice though). The same few lines below

A tip: in hook_exit, could you use drupal_write_record to simplify the code? It would automatically update or insert new line.

I am afraid this module might be a performance hog but I don't think it's up to us to judge that. I suggest fixing the t() issue and then good to go.

pgorecki’s picture

pgorecki commented
Status: Needs work » Fixed

Thanks a lot for the review and some tips, the code is fixed.

mermentau’s picture

mermentau commented

I don't think we can mark our own as status "fixed". Probably want to put "needs review".

pgorecki’s picture

pgorecki commented
Status: Fixed » Needs review

status changed to "needs review"

rteijeiro’s picture

rteijeiro commented
Status: Needs review » Needs work

Hi.

Please, add a README.txt file with the project description, installation, requirements, features...

Could you compare your project with similar projects that offers the same solutions, please?

I don't know why you have commented the hook_help implementation and why you use the hook_exit instead the hook_nodeapi.

Kind regards.

pgorecki’s picture

pgorecki commented
Status: Needs work » Needs review

Hi.

I added README.txt to the sandbox. I didn't find any project that offers the similar solution to my module (correct me if I'm wrong). However, similar functionality can be achieved with Rules+Flag+Views as described in this tutorial: http://drupal.org/node/405754, or with statistics module and custom code snippets: http://drupal.org/node/45286

I used hook_exit() intentionally, taking the same approach as is in the statistics module (http://api.drupal.org/api/drupal/modules--statistics--statistics.module/...). As a consequence, a node is "read" when user browses to node/X page, and will not be read if node is viewed in some other way (i.e. with Views).

rteijeiro’s picture

rteijeiro commented
Status: Needs review » Reviewed & tested by the community

Hi.

I have reviewed the project and it has clean, safe and well commented code. It works well and I think it's ready to become a full project.

Great work!!

Kind regards.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Reviewed & tested by the community » Fixed

Please remove the LICENSE.txt, as we do not allow those any more (one is provided with by the packaging).

Thanks for your patience - Git vetted user role granted.

Please review others' applications, because you know how painfully long this can take.

Thanks for your present and future contributions!

pgorecki’s picture

pgorecki commented

Thanks a lot everybody for tips, suggestions, and giving the time to review this module.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.