Refactor date handling

Haven't had a chance to look at this patch yet. But I stumbled upon a Drupal 5 version of jscalender in this thread: http://drupal.org/node/81691.

Also mentioned there is the jQuery version which I first had in mind, http://kelvinluck.com/assets/jquery/datePicker/.
That together with the jQuery time picker plugin might be interesting, http://www.texotela.co.uk/code/jquery/timepicker/.

Ok, I looked at the code, here are some comments:

1. $form['settings']['startdatejs'] can't be right?

2. I don't understand "/// Open polls with a startdate that is in the past but that need to be opened".

3. I prefer not to linebreak just to be < 80 characters, e.g line 288 and 819.

4. Using just a class with jQuery as in $(".edit-settings-startdate") makes it really slow as it has to go through the whole DOM - an id is a better option here.

5. We really have to be careful not filling up the settings box with too many options, already now I think it's too much. I have some ideas how I want the date functionality to work, to make it more concise (see below). But this is a difficult one. Just see how the settings box in Decisions look like.

7. I've attached a screenshot from google calendar, which I think could be a good start. Also see my previous post for jQuery plugins, they can be used here.

8. Finally, I think that maybe this patch could have staid a litter bit longer in the issue queue - not that it's so bad though. But it's a little bit sketchy and I don't mind reviewing/improving patches. I can see why you wanted this in quick though, as I have been bugging you about it :)

I think I understand what the "Open polls with a startdate in the past" code does, but won't it mean that old polls that has been manually closed will be re-opened?

Also, have you been thinking of implementing time? I first thought that you were going to add a field similar to "Authored by" which is really cumbersome to edit, but that we can later improve with some js. I didn't know about the date field though, but in general i think those select boxes are quite bad UI-wise. So my suggestion is to skip the checkbox ("use startdate") and the date select boxes in favour for a pre-filled texfield, similar to "Authored by". Thoughts on that?

My original mockup, http://drupal.org/files/issues/decision_startdate.png (I'd forgot about that) in http://drupal.org/node/88217 can illustrate my suggestion.

Do we really need to be rely on cron? If we could check to see if a vote is opened with some php logic instead, then "status" in the database could be used only for the manual setting. This would solve both #1 and #2.

I did play with the two plugins. They are not really ready yet. The datePicker paging is a bit slow and timePicker has some display issues and the implementation is not that good, it's a quite simple module though so it should be easy to fix. There won't be any problems to style the FormAPI the way we wan't it though, which you mentioned before.

I missed to comment some things:

I the statdate field is pre-filled with the current time as the "Authered by" field, isn't it the same thing as optional?

I should have pointed out earlier that I also favour an end date, the google calendar way is quite optimal me thinks. If you try google calendar you will also see an "all day" checkbox - we could have the exact same thing but for "unlimited".

No, my point was that we don't need a third status. If it's done correctly we don't need to store this in the database, the information is already there (startdate and (soon) enddate). The status active/closed is just an feature to give the user the possibility to easely close the poll without changing the dates - thus this just overrides the date checking. In a sense this feature could be removed, if we think the user is smart enough to set an endate that has passed to close a poll.

In Views to list active or closed polls you just use an argument that compares the today's date to the startdate and/or enddate. Without looking at Views I'm pretty shure this is already possible to do for e.g. "Authored by".

Isn't this done all the time in Drupal - comparing dates in the database to the current date? This isn't really any different, right?

Just a sidenote: I saw that Views use the textfield+jscalendar approach as date UI. I still favour a textfield instead of selects, it will also be easier for me to use as a base for a potential jQuery solution.

Attaching a patch which adds a missing parenthesis in advpoll_update_2().

We have to decide how to treat the status option. I'm leaning towards only a close option, and making it an "close poll" checkbox instead which overrides the duration. That might be more intuitive. Now it's not really clear what happens if the status is set on active and the startdate for example i set in the future. Before this patch the status was changed to closed in that situation, but that doesn't happen with this patch.

Will it be possible to ditch cron as discussed before?

As said before I like the textfield implementation as it gives us more freedom to improve the UI with js.

We have to make sure that it's obvious when a poll is opened or not. Hopefully the close checkbox will fix that.

Something that could help here and also clean up the UI as most users will not set a duration is to hide the start/endate when it's not defined, similar to how it was done before with the "Use start date" checkbox. An idea is to do this strictly with js (when the back-end is solid). An empty stardate in the db tells us that no startdate is set, i.e no default startdate i saved when the node is created.

Some comments on the current code:
* Endate doesn't have a time (maxlength = 10).
* A default endate is saved, it should be empty if not explicitly entered.
* endate has '#attributes' => array('class' => 'jscalendar') but not startdate

Great work as always!

"Keeping startdate/enddate optional and integrating it into a manual active/closed option is pretty tough."

To me it seems like this is just a matter of a helper function with a couple of if statements; i.e first check if the current time is within the set duration (if set) and if so, check if the poll is set to be opened. What have I missed?

The problem with *only* using the startdate/endate is that you miss the possibility to close a poll temporary that has a set duration. And I think it might be confusing to the user why we "mess up" the duration. UI wise I'm also a bit skeptical to more buttons.

Trying to move this forward.

* Still 3 database fields: enddate, startdate and active
* No cron, instead a helper function _advpoll_is_active($node) that checks if the poll is open, ie if today() > startdate and today() < enddate. It also checks that active == 1, if not the poll is always closed.
* The status option (which sets active status as before) is now a checkbox: "Close poll".
* Startdate and enddate can be empty.

I haven't dealt with the database things like the block and the polls page. Also, this patch is only ready for binary options. I just want an agreement on the implementation first.

Re-rolled.

1. Added back and improved the update path lost in previous patch.
2. Change order so endddate comes after startdate in queries etc
3. Added support for default duration.

Lowercase "int" seems to be used in core.

Todo:
* Display that a poll is closed (now it only displays that you are not eligable)

JavaScript (could wait):
* Some kind of checkbox(es) to show/hide dates.
* Always hide dates if "Close poll" is checked.
* Some kind of picker would still be nice, I don't know if the community has settled on one yet. Or we roll our own solution (http://labs.perifer.se/timedatepicker)

* Give default startdate same treatment as default enddate
* Removed extra space

I think I was wrong when in #29 when I said that "it only displays that you are not eligable". Or it was probably a bug. The current behavior is that the result is shown if the poll is not active. So we were wrong in http://drupal.org/node/143194, right? (We show the form even if they are not eligable, but no date support in that patch). Messy.

* Corrected _advpoll_is_active()
* Changed [setting][active] to [setting][close] at it makes it more obvious what we are doing there and made it work again

Maybe the best plan is the to re-open http://drupal.org/node/143194 when this gets in.

And the patch. I forgot to mention that I've added a check in _validate(), it's no used yet as explained above, but is useful when we fix http://drupal.org/node/143194 .

1. _advpoll_is_active() can now optionally return $status ('closed', 'pending', 'passed' or 'open'). Could need some doxygen.

2. Pending polls (startdate in the future) now renders the voting form with a message: "This poll opens in X days".

3. I removed the vote button instead of disabling it. I think it's better usability wise. Try it!

Sidenote: As both _voting_form() and _validate() are getting increasingly powerful more and more code are also identical in binary.inc and ranking.inc. We might benefit from trying to put that in some shared functions.

Re-rolled.

Thanks!

1. Shows the format
2. Validates dates (valid and endate before startdate)
3. Changed "in X minutes" to the actual date. Good catch!
4. Fixed spacing error.

The other stuff (usability improvements) feels like they can go in later(?).

I've been thinking on how we should use this new data more to improve usablity. Most of it I feel should go in a theme function, but we don't have a good one to put them in. Hopefully in Drupal 6 we can provide our own node-advpoll_binary.tpl.php, I think that might be the best option.

Refresh...

Added.

http://drupal.org/node/100098. I wanted to write a more inspiring commit message, but I was too tired. This is really great though.

Yep, tired. This one it is: http://drupal.org/cvs?commit=79661

On the date interval/cache issue: I remembered Typo (http://blog.typosphere.org) had a solution to this: The date is rendered in the html and then JavaScript is used to calculate the interval, thus working around the cache problem. Maybe even something for core Drupal.