CVS edit link for hippich

Hello,

I'd like to try again get CVS account for contributing to Drupal community. This time I bring recently created two small modules:
1) Time limit - http://www.yepcorp.com/time_limit.tar.gz
2) Discuss This! Views integration - http://www.yepcorp.com/discussthis_views.tar.gz

First one used to limit time anonymous users could browse site. After specified time user is redirected to login page (by default, could be changed in admin) and message shown, explaining that user should login or register before continuing use of site (message also configurable)

Second module just enhance Discuss This! module exposing it's table to Views module, so you can create block like "latest discussion about this article" easily.

Hope this time you will approve my application.

Thank you =)

Comments

pavel.karoukin’s picture

StatusFileSize
new1.32 KB
new1.15 KB
pavel.karoukin’s picture

Status: Postponed (maintainer needs more info) » Needs review

changing status to need review

avpaderno’s picture

Status: Needs review » Postponed (maintainer needs more info)
Issue tags: +Module review

Thanks for applying for a CVS account.

As we review only a project per applicant, you should choose the one you want to be reviewed. A more detailed description of the proposed project should also help to avoid the proposed project is reported to be a duplicate of another existing project, and speed up the application approval.

pavel.karoukin’s picture

Status: Postponed (maintainer needs more info) » Needs review

Ok. Please consider time_limit module as one used to apply for CVS account.

"Module used to limit time anonymous users could browse site. After specified time user is redirected to login page (by default, could be changed by admin) and message shown, explaining that user should login or register before continuing use of site (message also configurable)"

avpaderno’s picture

Status: Needs review » Needs work

The motivation should be expanded, and add more details about the module features; it should also compare the module with existing projects with a similar purpose.

pavel.karoukin’s picture

Hello,

This module was developed while working on client's site who wanted to keep site freely available for user only for limited time. So basically, anyone who wants to show site content for some limited time and then force users to register - could use this module.

For example, site providing reviews for some product category:
- User comes to site and browse all reviews available
- After 30 minutes he is redirected to page saying, that to continue browsing site he need to submit new review and register (this could be achieved with inline_registration module)

Another use - provide 'trial' period for new user to browse site and then ask them to register where they will need to pay for membership. The good point there - users do not have to register first to obtain 'trial' role. They obtains trial status right after they come to the site.

This time limit could be easily avoided by user, so this should be used on sites not relying heavily on this, but I believe 99% users will not bother with clearing cookies in order to get access to the site. And I believe this module will be used mostly on sites with free registrations.

Important part - search engines will crawl site freely since they do not store sessions and therefore all content from the site will be searchable in major search engines. So opposing to approach with registering user and giving him 'trial' role, in this case all content will be indexed by search engines.

I really can't find something comparable yet on Drupal.org repository - that's why I have developed it from the ground. If you could point me to something similar already existing - I will be glad to review this module and possibly contribute to it instead creating new module.

Regards,
Pavel

avpaderno’s picture

Status: Needs work » Needs review

The explanation is enough, for me; thank you.

avpaderno’s picture

Status: Needs review » Needs work
  1. The module defines its own Drupal variables, but it doesn't implements hook_uninstall().
  2. drupal_set_message( t(variable_get('time_limit_message', 'Please, register or login to continue useing of the site.')) );
    

    The first argument of t() must be a literal string; differently, the script that extracts the string to translate to create the translation template will not be able to extract the string, which would not be translatable (if not in the case another module uses the same exact string, but it's rather difficult it happens, when the string is dynamically changed).

  3. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.
pavel.karoukin’s picture

StatusFileSize
new5.22 KB
new1.58 KB

Hello,

I've updated module. both Coder module and code-style.pl didn't complained this time =)

not sure if I understood correctly regarding #2 tho.. Let me know if I am wrong.

New module attached and patch also there for easier reviewing.

Regards,
Pavel

pavel.karoukin’s picture

Status: Needs work » Needs review

forgot to change status...

avpaderno’s picture

Status: Needs review » Fixed
  1.   $message = t('!time_limit_message', array('!time_limit_message' => variable_get('time_limit_message', 'Please, register or login to continue useing of the site.')));
    

    It's useless to use t() in such cases; the strings passed as arguments for the placeholders are not translated. The string '!time_limit_message' contains just a placeholder; if the string contained two placeholders, then it would have made sense to pass it to t() because who translate the string can need to order them differently, or use a different character to separate the placeholders.

    In you case, don't call t(); the content of a Drupal variable is still translatable by adding a line like

      $conf['locale_custom_strings_it']['time_limit_message'] = 'Italian translation';
    

    in settings.php. The last letters after locale_custom_strings_ are the language code for the language used for the translation; the string can be translated in any languages.

  2. There is a string that is not changed, and that is not in sentence case (Time Limit Settings); you changed the others, bur forgot to change that one.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

avpaderno’s picture

Component: Miscellaneous » new project application
Assigned: Unassigned » avpaderno
Issue summary: View changes