This D6 module allows users to generate timelines based on the contents of your Drupal site. This visualization is based on content date fields assigned to content types on your site.

This is (somewhat) possible using exposed views, but exposed views does not scale on larger sites.

Users can also save generated timelines to easier find them later.

Project page: http://drupal.org/sandbox/staticred/1608484
Git: git clone --recursive --branch 6.x-1.x git.drupal.org:sandbox/staticred/1608484.git user_generated_timelines

Reviews of other project applications:
User-specific language: http://drupal.org/node/1591870#comment-6152394
Professional Share: https://drupal.org/node/1589220#comment-6330542

CommentFileSizeAuthor
#6 drupalcs-result.txt832 bytesklausi

Comments

patrickd’s picture

patrickd commented
Title: Drupal 6 module: User-Generated Timelines » User-Generated Timelines
Status: Needs review » Needs work

welcome,

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process. Report: http://ventral.org/pareview/httpgitdrupalorgsandboxstaticred1608484git

Remove the translations folder, translations are done on http://localize.drupal.org

Don't use such "// end usergen_timeline_uninstall();" comments (please)

Functions that are hooks have to be commented with

/**
 * Implements hook_HOOKNAME().
 *
 * What your doing with it.
 */

Generally, please have a look at drupal coding standarts

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

staticred’s picture

staticred commented

Thanks Patrick, I'll run through and make the coding standard changes.

staticred’s picture

staticred commented
Status: Needs work » Needs review

Made changes based on Patrick's suggestion, and results of PAReview.

See http://ventral.org/pareview/httpgitdrupalorgsandboxstaticred1608484git

Please re-review.

sanchi.girotra’s picture

sanchi.girotra commented
Status: Needs review » Needs work

1.Please provide git link for non maintainer in the issue summary.
2.There are still files other than README.txt in the master branch, make sure to remove them.
Manual Review:
1.Suggestion:In .install file there is no need of usergen_timeline_install() and usergen_timeline_uninstall() to install-uninstall schema.As the tables declared by hook_schema will be automatically created when the module is first enabled, and removed when the module is uninstalled refer http://api.drupal.org/api/drupal/developer!hooks!install.php/function/ho....
2.Write usergen_timeline_uninstall() in .install file to delete all the variables used in .module starting with 'usergen_timeline_%'.
3.In usergen_timeline_perm() defined permission 'Administer usergen_timeline' which is not used in the .module file.

staticred’s picture

staticred commented
Status: Needs work » Needs review

Hi Sanchi - some responses:

> Please provide git link for non maintainer in the issue summary.

The git link is provided in the summary already.

> In .install file there is no need of usergen_timeline_install() and usergen_timeline_uninstall()
> to install-uninstall schema.As the tables declared by hook_schema will be automatically created
> when the module is first enabled, and removed when the module is uninstalled

This is incorrect - according to http://drupal.org/node/146862, hook_install and hook_uninstall are required for Drupal 6.x. These are not required for Drupal 7.x, which is where I think you're getting confused.

> Write usergen_timeline_uninstall() in .install file to delete all the variables used in .module starting
> with 'usergen_timeline_%'.

This is weirdly at odds with your previous suggestion that the hook_uninstall() isn't required. I'm going to ignore this, partially because the function's already there, and partially because I have no idea what you're actually suggesting.

> In usergen_timeline_perm() defined permission 'Administer usergen_timeline' which is not used
> in the .module file.

This is used in the usergen_timeline_menu() function to control access to admin settings for the module.

As a note for followup, please don't review this module using Drupal 7.x conventions and standards, as this module is explicitly targeted for Drupal 6.x.

staticred’s picture

staticred commented
Issue summary: View changes

Adding reviewed project information

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: +PAreview: security
StatusFileSize
new drupalcs-result.txt832 bytes

Sorry for the delay. We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 6.x-1.x branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. usergen_timeline_schema(): this is a hook and should be documented as such. See http://drupal.org/node/1354#hookimpl . Same for your other hook implementations.
  2. usergen_timeline_perm(): permissions should be all lower case.
  3. usergen/timeline/save and usergen_timeline_save(): This is vulnerable to CSRF as this is an action link, there is no token and there is no confirm form. Please use the Form API to create data. See http://drupal.org/node/178896
  4. usergen_timeline_retrieve_data(): do not concatenate strings directly into SQL statements, always use placeholders to avoid SQL injection. See http://drupal.org/writing-secure-code
  5. usergen_timeline_retrieve_data(): $description is build from user supplied data and needs to be sanitized before being printed to the user. Otherwise you get XSS exploits. Or does the receiving javascript do the sanitization?
staticred’s picture

staticred commented
Status: Needs work » Needs review

Thanks klausi - two reviews down, one to go for helping you out. :D

I've addressed the pareview.sh issues, as well as as the manual review with the following exceptions.

3. The save link is meant to be seamless, so a confirm form would not be desirable. I've added in tokens to prevent against CSRF as suggested by the link you included.

5. $description is coming only from the site's content editors, so a certain level of trust in that content was implied. That being said, the data in $description gets sent to the timeline module, which sanitizes it before display.

wolvern’s picture

wolvern commented

Hi, staticred

I've looked at your module and found these problems/suggestions:

In the settings page, I would make "Additional fields to filter by:" list resizable. If one has a lot of fields to choose from it's a bit of a pain to scroll in such a small list.

Also in the same page your "Label" and "On-screen help" fields are both wrapped in <a href="/usergen/timeline"> for some reason.

usergen_timeline.module:128: there's an unused variable ($helptext)

In your demonstration site no images are visible. Although that website doesn't seem to be dedicated solely to your module, or perhaps you are not its' maintainer, so maybe it's not relevant in this case, I'm not sure... :)

And finally, I was unable to see the generated timeline and review further due to a Fatal error: Call to undefined function usergen_map_getfields() in /sites/all/modules/user_generated_timelines/usergen_timeline.module on line 1001

wolvern’s picture

wolvern commented
Status: Needs review » Needs work
staticred’s picture

staticred commented
Assigned: Unassigned » staticred
Status: Needs work » Postponed

Thanks Saulelis - I'll be rewriting this module to integrate a user-generated mapping tool, and will incorporate your suggestions.

I'll resubmit after this work is done.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Postponed » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

Adding review to body