More precise closing date by defining a start date.

There's a discussion on the Quiz module that I'm following and I'd probably want to use the solution that they decide on: http://drupal.org/node/27133

If we're going to add a flexible start date I think we should just change duration to a true date as well. It's fairly common for an election to run from 8am one day to 5pm X days later, so we might as well support that case. But in the short term we can just stick with the current duration setting until we get a start date working.

Yes, the current duration setting is based on the "Authored on" date for the field.

Here is a first patch trying to fix this issue. Here are its main features:

1. Replace the "duration" select box by a "Closing date" widget.
2. Add a "Start date" widget.
3. Does not change the current fields in the DB whatsoever.

The plan is to reintegrate the duration as an option later on (like, you'd be able to choose between a closing date or a duration). This patch merely handles the saving/loading of those values: it still does not affects the behavior of the decision.

Here is the include file needed by patch #1.

Thanks for the patch! Anarcat is testing it (and will probably commit it), but if you could get cvs access, it would be great. (Yep, this is a formal invitation to get Tatien a cvs account).

The first thing that strikes me is that the date fieldset is a bit overwhelming... Two fieldsets, within a date fieldset within a Decisions settings fieldset... that's too much. Also, the date should probably use the date type from formapi.

More importantly, the date settings do not include the notion of "no end date". That means that by default, if validation would be implemented, it would be impossible to vote on new decisions. That needs to be fixed.

A few cosmetic fixes in the code all around:

* the .inc file should have an empty $Id$
* header comments should be enclosed in /** @file */ in the .inc file (see decisions.module for example)
* the .inc file should be included (and only include_once) in the _menu() hook (as decisions_modes() is called)
* there are too many XXX and debugging/commented code around. for example, the collapsible parameter should work, see decisions node settings for an example.

Finally, I would very much like to see this torn in a seperate module. The .inc file is a good start, but it could go further: we need to add _is_valid() hooks that would also work for quorum. The _date module could also use a _can_vote() hook that would deny user's right to vote after a decision is closed.

I agree with you that the date fieldset is a bit overwhelming, but the problem here is that there's no such thing as a "datetime" type in formapi. I thought the dates should at least include hours and minutes.

The way I do it the same as in the event.module and also gets inspiration from the quiz.module. The reason why there's a "dummy" fieldset is that it seems to be the only way to specify a "#title" for a custom field such as this one.

I've noticed that the "collapsible" bug only arises when the non-collapsible fieldset is enclosed within a collapsible one, as is the case. I've filled a bug issue for that.

I agree that there should be a "no end date" option. However, I believe it would be better to commit it in a separate patch. Now that I got CVS access, things will be alot easier.

I also agree that we need to find a common ground on the "valid" and "can vote" concepts, but I hardly feel the necessity to have them as "hooks" in separate modules. I'm just feeling like we won't really need any other modules than "quorum" and "date" that would implement those hooks. If we're making hooks for just two modules, it's just not worth it IMHO.

I suggest that for now I just commit a modified version of this patch where the date options would not be a fieldset within a fieldset i.e. exactly like it is in the event.module. I'll also fix the cosmetic issues you mentioned in your * dotted list)

The next patch will be a "no end date" checkbox functionality.

And the patches after will actually implement the "valid" and "can vote" concepts.

After that, I'll try to find a cleaner way for the fieldset/datetime issue but as I mentionned I'm afraid it might be difficult. I'll look how they do it in the CCK "date" module, but anyway my opinion over this is that (1) there should be a "datetime" type within the formapi (webchick has already filled an issue about that on nov. 13, it's still left unanswered) and (2) there should be a "dateapi" module in Drupal. However I hardly have any control over this for now.

Sorry, I forgot to add that a next patch will also offer the option to choose a "duration" instead of an "end date", to answer what was brought in by fajestarter:

"I like the simplicity of the current duration drop-down, so I also like to keep it (i.e. not replace it by a date field)."

Commited a first patch, see CVS log #46081.

Commit a second patch, see CVS log #46085.

Wow... you sure didn't waste time. :)

While I'm glad you have CVS access and can now commit without having me to review and commit everything, i still have comments on the patchset.

* as i said before, the include of the .inc should probably sit in the _menu(). look at how decision modes are included... no code should be ran when the module is included, but only when certain functions are called
* remove debugging code from your commits. there are drupal_set_messages() in the date patch. there's also commented code left: either comment why it's still there or remove it altogether.
* shouldn't DECISIONS_RUNTIME_INFINITY be "0" (as it was before, btw)?
* i hope you tested this better than the first patch

As for my "everything as a module" thing, I think that there is room for putting those things as a module. The advantage is that by default, the settings fieldset would be less crowded, and that it would be possible for admins to disable the thing system-wide. Now, the question wether it's worth it to create hooks for one or two module doesn't really apply because it's the only *way* to make communication with a module work at all. :)

For me, the first step would be to split quorum and the date functions in a seperate .inc (or a .module rightaway). I can take care of the later modularisation and testing.

Thanks.

Commited new patch, see cvs log #46096.

Hi anarcat, let me answer a few of your questions.

* as i said before, the include of the .inc should probably sit in the _menu(). look at how decision modes are included... no code should be ran when the module is included, but only when certain functions are called

I tried, but then I'd need to call the functions with a call to call_user_func(), which I found rather cumbersome. The .inc only defines some functions, no code is executed whatsoever.

* remove debugging code from your commits. there are drupal_set_messages() in the date patch. there's also commented code left: either comment why it's still there or remove it altogether.

Yes I know :), and I submitted a patch for this, see log #46086.

* shouldn't DECISIONS_RUNTIME_INFINITY be "0" (as it was before, btw)?

Sorry, I didn't know how it was before, I thought it was "-1" ;) Should have looked better. Anyway, I'm hesitating for this: either we use "0" (but then its much error-prone) or we use the max number of the field (which, BTW, I now figure out I have wrongly set to 1000000000, should be 9999999999 since the field is an int(10)). What do you prefer?

* i hope you tested this better than the first patch

I tested it the better I could, but it'd be good if you could cross-check it.

I'll stop working on that for now and let you do some work on the modularisation if you will.

I don't see how including in the hook_menu() forces you to use call_user_func().

I'd rather have INFINITY set at 0, cleaner, and DB-independent.

Thanks for the debugging code cleanup. Don't stop for me, I won't work on decisions today, but watch the clock.

Commit new patch, see CVS log #46120.

I changed DECISIONS_RUNTIME_INFINITY to zero in my last commit.

I think this works properly now.