This is a javascript multiselect calendar widget using the [MultiDatesPicker](http://multidatespickr.sourceforge.net/) library on top of the date_popup module.

It allows you to select multiple dates (with no time) in just one field, instead of having to add multiple popup widgets.

Link to sandbox page: https://www.drupal.org/sandbox/jackbravo/2417391

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jackbravo/2417391.git date_multiselect

Parereview: http://pareview.sh/pareview/httpgitdrupalorgsandboxjackbravo2417391git

Most of the errors are on the multidatespickr.js file which is an external project that is included. The multidatespicker library is GPL.

CommentFileSizeAuthor
#21 Screenshot 2015-04-30 17.17.57.png84.21 KBandrefy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

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.

rivimey’s picture

@jackbravo thanks for contributing.

The pareview checks are failing in a number of ways. Please fix all issues.

You have added a 3rd party library inline in this module, which is very discouraged : see https://www.drupal.org/node/422996 : "In general 3rd party libraries are forbidden". I don't see anything in the exceptions to that rule which would apply here.

I suggest you :
- include a hook_requirements implementation to check if the JS is in a ./libearies folder and complain if not. Many other modules do this...
- include instructions in the readme indicating where the JS is available and if there are any version restrictions.

rivimey’s picture

Status: Needs review » Needs work

Automated Review

Needs work

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation. Note however the old module https://www.drupal.org/project/jquery_multiselect
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
No: Does not follow the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
The readme file is lacking detail and lacks important sections.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
No: Does not meet the security requirements.
Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
  1. (+) It would help to have hook_help(), hook_requirements() implementations
  2. (+) Drupal code style has not been followed, with long lines, inappropriate indentation, and for many cases no function comments.
  3. (*) There seems to be a lack of format checking: calls to check_plain() et al. Recall that Drupal DB values are stored unchecked.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

jackbravo’s picture

Status: Needs work » Needs review

Thanks for the review! =).

About the 3rd party library I didn't give much thought about it. I based my code on the date_popup module code, which does the same for the http://plugins.jquery.com/project/timeEntry plugin: it is included with the date module. And I can think of other couple examples: colorbox, bootstrap, lightbox. Anyway, I can see that new modules do as what you suggest: chosen, jquery_ui_multiselect_widget. So now I use the same approach as the chosen module and we now need a manual download of the multidatespicker jquery plugin. README has been updated.

You added a secure code section, but I don't see how I'm not complying with the specifications. I use t() where a translation is needed, but I don't save any input to the database in my module but instead rely on other modules for that (date and the field API) so I don't think I need to use filter_css or check_plain.

What else would be needed? And thanks again for the review!

mlmoseley’s picture

My understanding is that if you are using a third-party library, you need to make it a library off of sites/all, and access it that way. It doesn't matter what other projects have done or were permitted to do -- that is current best practice. You can create your own version of the the library that includes the multidatespicker jquery plugin and provide it with the module.

https://www.drupal.org/node/1342238

mlmoseley’s picture

And the git clone command above requires your password.

You need to post the the public git clone command. On your sandbox page select the 'version control' tab, then deselect 'maintainer', then click on the 'show' button, then copy and paste the git clone command.

jackbravo’s picture

Issue summary: View changes

Hi, I'm sorry. The version control tab on the sandbox seems to give me the private clone command. I signed out and copied the public instead.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jackbravo/2417391.git date_multiselect

And you are right, about not including the javascript library on my module I'm doing that now. The javascript file is not included and requires a manual download to the libraries folder, which is described in the README.txt file.

e.bogatyrev’s picture

Status: Needs review » Needs work

@jackbravo thank you for contributing.

There are some issues I found:

date_multiselect.js

  1. line 19 - missed ';'
  2. line 11 - settings should be defined as variable (var before definition)
  3. line 32 - what for did you place the function getMinDate() separately? It will be better if it placed inside of Drupal.behaviors.date_multiselect as second function - also in this case you can continue use $ instead of jQuery definition.
  4. line 34, 36 - duplicate declaration of minDateObj

date_multiselect.module

  1. line 36-39 - you can place all settings into $info['date_multiselect'] without merging.
  2. line 82 - you use static variable to be sure that you can't include libraries twice. But this logic is already implemented in drupal_add_library, drupal_add_js (these functions use drupal_static). What for to do duplicates?
  3. line 99, 125 - absolutely unnecessary define arrays separately, they will be created in the first definition.
  4. line 169, 170 - use drupal_static instead of static (https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal...)
  5. line 266, 267 - comment is too long at one row, please split it and place part of it to next rows.
jackbravo’s picture

Status: Needs work » Needs review

Thanks a lot @e.bogatyrev. I've fixed all the issues. Many where because of old stuff from the module this is based on that should be fixed as we eliminated unnecesary stuff (date_popup).

PA robot’s picture

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

Project 1: https://www.drupal.org/node/2417425

Project 2: https://www.drupal.org/node/2468435

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.

zbombicz’s picture

Hi jackbravo,

It will be a very helpful module. Unfortunately I found some mistakes, errors. Please check them out.

Automated Review

pareview:

FILE: /var/www/drupal-7-pareview/pareview_temp/js/date_multiselect.js

------------------------------------------------------------------------

FOUND 2 ERRORS AFFECTING 2 LINES

------------------------------------------------------------------------

6 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0

44 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements. / No: List of security issues identified.
Coding style & Drupal API usage
  1. (*) Undefined index notices (minDate, maxDate,...). date_multiselect.module line 55, 61. I recommend adding default values to hook_field_widget_info.
  2. (*) Misleading labels for fields Minimum and Maximum date relative to the current date. The label concludes, that I have to add dates, but in fact I need to add numbers of days. I recommend providing field description to explain operation.
  3. (*) The module is not working if I choose collect end date. Also it is weird, that I can check Hours, minutes, seconds option, if its not treated. If these options are meaningless in your module, I recommend disable these options, or make extra validations for checking faulty configuration (in case of Multiselect calendar widget).
  4. (+) The provided (info file) configuration page admin/config/date/date_multiselect doesn't exists.
  5. Readme.txt: Add external multidatespicker library into Requirements section. https://www.drupal.org/node/2181737:

    "The requirements section (required) shall make it clear whether this project requires anything outside of Drupal core to work (modules, libraries, etc)."
  6. Typos: realtive
  7. Add argument documantation for functions: date_multiselect_date_format, date_multiselect_implode_dates.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

jackbravo’s picture

Hi @zbombicz!

Thanks a lot for your review.

1. (*) Undefined index notices (minDate, maxDate,...). date_multiselect.module line 55, 61. I recommend adding default values to hook_field_widget_info.

Fixed.

2. (*) Misleading labels for fields Minimum and Maximum date relative to the current date. The label concludes, that I have to add dates, but in fact I need to add numbers of days. I recommend providing field description to explain operation.

We added a description to the field to explain how to use it.

3. (*) The module is not working if I choose collect end date. Also it is weird, that I can check Hours, minutes, seconds option, if its not treated. If these options are meaningless in your module, I recommend disable these options, or make extra validations for checking faulty configuration (in case of Multiselect calendar widget).

We implemented hook_date_field_settings_form_alter to alter the field form when using the date_multiselect widget type so we don't show the enddate field and set the granularity to ['year', 'month', 'day'].

4. (+) The provided (info file) configuration page admin/config/date/date_multiselect doesn't exists.

Fixed

5. Readme.txt: Add external multidatespicker library into Requirements section.

Fixed.

6. Typos: realtive

Fixed.

7. Add argument documantation for functions: date_multiselect_date_format, date_multiselect_implode_dates.

We removed this functions and instead we are now using a DATE_MULTISELECT_FORMAT constant. Since we don't show dates on the field to the user, the #date_format element configuration was not needed.

mvdve’s picture

Simple but very usefull module. Still some issues that need to be fixed.
First, pareview has found some small issues regarding coding standards.

See the pareview page.

Camelcase and underscores are used for variables. Within Drupal only underscores are used. See the Drupal naming convention.

A suggestion: When using external libraries, it is a plus to use the libraries API.
This gives you a very simple API to load and detect the library in sites/all/libraries.

Load and checking the library can be as simple as:

  if (($library = libraries_load('library_name')) && empty($library['loaded'])) {
    // Set error.
  }
mvdve’s picture

Status: Needs review » Needs work
jackbravo’s picture

Status: Needs work » Needs review

Ok, so I'm now using the libraries module to include the library. I also implemented a hook_requirements functions to let know the user if he doesn't have a correct version of the library.

I'm not sure what camelCase variables you are referring to. Could you provide a line number or function name? If you are talking about the names in the settings array in date_multiselect_element_process(). Those are camelCase because they will be translated into javascript variables, and I think this is the way it is used in other places (like date_popup).

I fixed one of the pareview issues. Two indenting issues remain, but I'm afraid that fixing them would mess up the version control information for that file (I would need to change almost all the lines in that file). So I think it is better to leave it like that. This error is carried over from date_popup, which was the base module for this one. Or what do you think?

rivimey’s picture

Hi, Just re-run the pareview tests and it shows a bunch of issues on the .install file as well as the two indentation issues I think you refer to. I can cope with the JS indentation issue but reindenting a file shouldn't be a problem, really.

I suspect the camelCase that the other reviewer mentioned is in the names of the settings - for example, yearRange at 151, dateFormat, firstDay at lines 256 & 257, maxPicks at 265. As you have not consistently used camelCase for these (see for example year_range at 251), it would be better to be consistent and for better or worse the Drupal standard is for underscores. I am no JS expert though so would suggest seeking advice from someone like @klausi.

The new code for requirements looks reasonable, though the description attribute line must be broken up: the line is far too long. You should probably make it 5 or 6 lines, using intermediate variables. You may be able to extract common elements to some degree.

I'm concerned about the DATE_MULTISELECT_FORMAT being hard-coded to USA style; other countries don't use that style. If it must be hardcoded, then use the ISO standard (YYY-MM-DD) but much better to make it configurable.

It's good that you've considered a responsive design (JS file, line 16), but hardcoding 380 pixels for the window width is not that helpful; fixed numbers make assumptions. Could that be configurable, or a way found that makes it responsive without assuming fixed values?

jackbravo’s picture

Hi @rivimey, thanks for the review =)

You are right, I did messed up with the camel case and underscore syntax! I fixed the wrong year_range variables. According to drupal documentation seems like camelCase is preferred for JS: https://www.drupal.org/node/304258#drupal-settings

About the pareview. I fixed all the issues. Reindenting a file is an issue when you want to use git blame later to know who changed a lined, or on what commit it was changed. But luckily enough it is an issue common enough that seems like there are solutions:

- http://stackoverflow.com/questions/3945382/git-commit-that-doesnt-overri...
- http://makandracards.com/makandra/13245-git-blame-how-to-ignore-white-sp...

So I got ahead and fixed it.

We added DATE_MULTISELECT_FORMAT as a constant instead of a user defined variable like variable_get because in reality the user never sees it. He uses the widget instead. So I don't think it is necessary to change it.

About camelCase in JS. I found nothing specific in https://www.drupal.org/node/260140

And finally, we changed the hardcoded 380 value to a variable_get() call function.

What do you think?

Thanks again!

andrefy’s picture

hey jackbravo

I tested this module from a fresh Drupal installation. I think should be wort to add the date module as a requirement on the info file

dependencies[] = date

I would expected that data_popup module will bring the date dependency but it doesn't, it brings date_api, and adding date as dependency make sense the widget is mean to be used on a field and also is under the readme.txt file.

Thanks the contribution, it is pretty useful, I would have used it on several previous projects.

Regards

jackbravo’s picture

I added the dependency as you suggested. You are right, date_popup doesn't depend on date because it can be used as a stand alone widget only for views and not fields.

Thanks for testing @andrefy =)!

balis_m’s picture

It' s really interesting module. Make date selections more users friendly.

I tested on the latest Drupal version and it works good.

I agree with @andrefy that the date module must be required and not the date_popup.

andrefy’s picture

Maybe the date_popup still needs to required since this module is build on the top of date_popup.

This module is working fine for me for Field Types:
- Date (Unix timestamp)
- Date (ISO format)

But I am having issues with the Field Type:
- Date

Steps to reproduce issue
- On a content type create a field type date and assign the Multiselect calendar
- Add a new content from that content type

After follow that steps I get the following error.

Issue Image
Issue image

It seems that something is missing, the Pop-up calendar seems to work fine with the Date field type. Based on the error seems that is is always using a unix time stamp format instead of a date format.

Regards

andrefy’s picture

Status: Needs review » Needs work
jackbravo’s picture

Status: Needs work » Needs review

Thanks for the review @andrefy!

I finally got some time to work on this and I fixed the problem. Ineed I had only tested using a unix timestamp date format. But now it works with all 3: date, date iso, unix timestamp. Only 2 functions needed changing =).

ajalan065’s picture

Drupal already has a module named Datepicker which performs the task as is mentioned in your module.. Then what extra benefit do your module gives???

jackbravo’s picture

Hi ajalan065. The datepicker module and the date_popup module allow you to use a date javascript widget to select a date, but they only allow you to select one date per widget. So if you set up a field that allows multiple values, you will use several widgets to select multiple dates. Using this module you use only one widget.

In the [module page](https://www.drupal.org/sandbox/jackbravo/2417391) it says:

> It allows you to select multiple dates (with no time) in just one field, instead of having to add multiple popup widgets.

But maybe that is not clear enough. I added a screenshot to also show what the module does. Tell me if that helps. And if you have any suggestion on how to improve the description I'm also all ears =).

ajalan065’s picture

This module works fine by me

DevElCuy’s picture

Status: Needs review » Reviewed & tested by the community

Great work @jackbravo!

cweagans’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your contribution!

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:

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.

jackbravo’s picture

Yay! Thanks a lot to all the reviewers!!!

https://www.drupal.org/project/date_multiselect =D.

Status: Fixed » Closed (fixed)

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