A simple module that display list of users birthday and optional sends notifications about users birthday to the specified e-mail address.

You can:

  • View list of users with filled birthday field.
  • Configure module to send notification about user birthdays in specific period.

Sandbox:
https://drupal.org/sandbox/sygnetica/2166513

GIT:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sygnetica/2166513.git birthdayinform

Preview results:
http://pareview.sh/pareview/httpgitdrupalorgsandboxsygnetica2166513git

Comments

sygnetica’s picture

Issue summary: View changes
sygnetica’s picture

Issue summary: View changes
PA robot’s picture

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.

zhuber’s picture

This module enables correctly on my local sanbox and passes PAReview, so it is almost ready in my opinion.

There are a few things I would like to see changed for the configuration form (these are all minor changes):

  1. Date format should use the drupal date formats that are content managed and provided by core at "/admin/config/regional/date-time/formats"
  2. Wording is off for various sections:
  • "DAYS OF WEEK TO CHECKING USERS BIRTHDAYS IN DEFINED NEXT DAYS AND SENDING E-MAIL NOTIFICATION"
  • "Number of next days to checking"

I would also remove the notification email address and use each user's email address instead. This would allow for the site to automatically offer birthday greetings and special birthday offers to it's members.

jmaerckaert’s picture

I tried to install your module but I received the error:
FieldException: Attempt to create a field of unknown type datestamp. in field_create_field() (line 110 of C:\wamp\www\stock\modules\field\field.crud.inc).

Otherwise I agree with @zhuber.

jmaerckaert’s picture

Status: Needs review » Needs work
sygnetica’s picture

@zhuber

There are a few things I would like to see changed for the configuration form (these are all minor changes):

Date format should use the drupal date formats that are content managed and provided by core at "/admin/config/regional/date-time/formats"
Wording is off for various sections:

"DAYS OF WEEK TO CHECKING USERS BIRTHDAYS IN DEFINED NEXT DAYS AND SENDING E-MAIL NOTIFICATION"
"Number of next days to checking"

Resolved

I would also remove the notification email address and use each user's email address instead. This would allow for the site to automatically offer birthday greetings and special birthday offers to it's members.

This functionality will be in next version.

sygnetica’s picture

Status: Needs work » Needs review
larsdesigns’s picture

Status: Needs review » Needs work

MANUAL REVIEW
=============
1. Please include configuration instructions in the README.txt file.
2. For line 11 in birthdayinform.module, when a switch() just has one case, it should be an if statement. Otherwise the code looks reasonably good to me.
3. On line 94 for function birthdayinform_form_user_register_form_alter(), Using unset() will likely cause errors. Check your recent logs report. Instead, use #type hidden to hide the field. Here is a link to the documentation: https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
4. For the birthdayinform.admin.inc file, I recommend writing validation functions for the form fields using #element_validate to call the validation function. For example: $form['checking_config']['birthdayinform_number_of_days'], you should probably check that the user enters only numbers and the format of which they are entered. Use form <?php form_error($element, t('This field is required.')); to return an error when the check fails.
Here is the URL for the Form API documentation: https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
5. For fields that should be required: Use #required to set the red asterisk and validation. Looks like on line 65 in the birthdayinform.admin.inc $form['checking_config']['birthdayinform_last_send'] wants to be required.
Here is the Form API documentation that includes an example. https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

AUTOMATED REVIEW
================

The automated review only turned up some minor issues. Which looks good. For the white space errors, look for a feature in your text editor or IDE that automatically removes trailing whitespaces. This is super helpful.

Look under Remove spaces section for Komodo IDE as a random example: https://drupal.org/node/248377

http://pareview.sh/pareview/httpgitdrupalorgsandboxsygnetica2166513git

Submitted by Anonymous (not verified) on Fri, 01/03/2014 - 15:48

Review of the 7.x-1.x branch:

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

FILE: /var/www/drupal-7-pareview/pareview_temp/birthdayinform.admin.inc
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
21 | ERROR | Whitespace found at end of line
27 | ERROR | The $text argument to l() should be enclosed within t() so that
| | it is translatable
27 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/birthdayinform.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
286 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------
Codespell has found some spelling errors in your code.

./birthdayinform.module:280: formated ==> formatted

sygnetica’s picture

@larsdesigns thanks for review and for the clue about automatically removes trailing whitespaces. It is very usefull.

1, 2, 4. Done.

3. This alter function don't unset whole field but "#default_value" and "#date_items" only. This is to not confuse the user by field filled default date.

5. birthdayinform_last_send variable is to compare today date with last send date in hook_cron and not send the email several times a day but its not required. It's filled up by module after sending email.

http://pareview.sh/pareview/httpgitdrupalorgsandboxsygnetica2166513git

sygnetica’s picture

Status: Needs work » Needs review
majorrobot’s picture

Hi sygnetica,

First of all, this is some pretty tight code! I can tell you've spent some time refining it. Also, I learned a new PHP function while reviewing — strnatcmp(). Good one for the arsenal.

I have a few questions and concerns, which I'll break down below.

Security

I found no security issues.

License

Good — no license.txt or 3rd party libs included in the repo.

Repo

Good - project page and git repo links are here. Developer is working off of a 7.x-1.x branch. Everything seems up to spec.

Duplication

I see a potential issue here. This module appears to replicate the functionality of the Birthdays module. sygnetica, could you provide some information here about similarities/differences and what sets the modules apart?

Documentation

The module provides a helpful README and code provides enough commenting in my opinion. However, the modules uses DateTime::diff, which requires PHP 5.3. You will make many users very happy if you warn them ahead of time that your module will have issues on their older versions of PHP. :)

Coding standards

Only a few small issues:

  1. Line 12 needs t() around the string.
  2. Seems a bit overkill to create 7 separate variables for birthdayinform_checking_day_N. Consider making this an array (which Drupal will serialize before storing).
  3. Lines 227-231 and 236-240 repeat code. I recommend wrapping this into a separate function or rearranging the logic to form these in the same lines of code.

Other issues

  1. When registering, I get the following error:

    Notice: Undefined index: #date_items in date_combo_validate() (line 439 of [...]sites/all/modules/date/date_elements.inc)..

    I suspect this is related to larsdesigns's #3, above. Something about your unset() is making the date module unhappy.

  2. Regarding larsdesigns #5 above, I suggest removing the birthdayinform_last_send field from the form altogether. I can't think of a great use case for the admin needing this, though I can see how it is very helpful for development purposes. Otherwise, it seems like it could be confusing for an admin.

Thanks so much for contributing!

majorrobot’s picture

Status: Needs review » Needs work
PA robot’s picture

Status: Needs work » Closed (won't fix)

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.