Implement support for Date popup module on Ticket and Invoice Date Fields

After looking at the code it looks like there's code missing for the date_api and date_popup functions in the ticket. The code is present in project and task.

Are you sure you're using version 1.9? The current version is 1.32.

I can create a patch for this if you want.

I have a patch against version 1.32 (which will also work on -dev) once confirmed it's the right version.

One problem I do see (which might be why it wasn't implemented before) is that in the ticket and invoice, the drop-down boxes allow you to put a blank date in the fields). I'm not sure if the date_popup module allows a NULL value (I'm still looking).

Does anyone else have any comments on this?

I suspect the reason is simply that the person who contributed the code needed if for project and task only.

OK. I nearly have something for 1.32
The only issue I see is that it shows 1-1-1970 for any date which is meant to be blank.

This is because the database holds a 0 (zero) for that date.

Not quite sure where to fix it. I suppose, if date=0 then display nothing??

Here is the file so far...

Its for ticket and invoice.

I also have a version for -dev too, if you want it.

When date_api and date_popup are enabled, do the date fields appear twice? From a read through, I can see additional form elements added, but not taking away the original ones.

The date fields don't appear twice. The second form element over-writes the first one.
It's the same code already in place in Projects and Tasks.

The only thing I'm not sure about is in relation to my comment on #6.

Any thoughts?

idea to #6

when displaying or loading the date, check if date is empty or 0. if empty set the value to NULL. If the value is Null no date should be displayed (i am unsure about that, but else we can put an if(!empty()) in the theme function

I can do that but at the moment i am on the drupal camp in munich

OK. I've added the 0/NULL check for the ticket and invoice date checks (not the other node types).

FYI, if the dates are left blank on the other node types in Storm, it also shows 1970-01-01.

This also needs fixing.
Should we fix with this issue of another?
I would prefer another one (on the -dev version).

Would one option be to run a database update to set all "0" dates to NULL then we could assume that "0" will never be present?

I was wondering that but it would mean changes to some core storm functions to return NULL instead of 0 for ALL date fields.
I could go through and do it but I think it should be done under a new issue. It's a bigger job.

I could open a new issue, finish the changes needed here and then work on the global changes, if people want.

Sounds good.

So RTBC for patch #7 with the zero issue being dealt with separately.

New issue

#794638: Replace 0 with NULL for all date related fields.

Committed, thanks.

Question - How is this put into the latest -dev version too?

It has been committed to CVS - which then rolls a new -dev release at midnight and midday.

In continuing with #567558: Extend auto-billing to other types of Storm node I think this patch might be causing a problem with creating invoices from other nodes (including timetracking). Can you test this and confirm it's a problem?

I create a timetracking record and then add to new invoice. The requestdate and duedate are not set because of this code now.

Let me know.

Is it possible to rollback this change?
I'll look at it again.
This could be the reason why the invoicing dates were not changed before.

Rolled back.

Back to needs review.

It seems that the code from #7 is still in the code.

Could this also be removed?

Sorry, should be sorted now.

looks good now. I'll rework the solution.

Urgent - I've noticed that the code for this patch in STILL in v1.33 (current version) and it's causing problems for a client of mine.

The changelog shows it was NOT added but I see the code.

Can you get the code removed from all versions (1.33 and -dev, if still exists).

Thanks.

I've changed this to critical, only to get the code removed from 1.33 as it should have never got into the live version (it was rolled back, I thought).

I may have committed the code by accident with the SA patch, but will have to check.

There is no way of removing the code from 6.x-1.33. It would have to be removed from -dev, which then is rolled out as 6.x-1.34.

I'd suggest that you patch your client's site since there will not be another official release for another month or so. Not sure if reversing the above patch will work considering the changes since.

Actually, it looks like my error was when I tagged for 6.x-1.33. The code must not have been totally up to date.

From my browsing of CVS, it looks like your best option would be to upgrade your client's site to 6.x-1.x-dev, until 6.x-1.34 is released. 6.x-1.x-dev does not seem to contain this code.

Could someone confirm that the -dev version works for this now?
When creating invoices (manual and auto) and tickets that the date is shown/stored correctly?

Also with and without using the Date Popup module.

I'll test it too but I'd like independent confirmation too.

Thanks

Tested with and without date popup - and seems to be working for me.

OK. Now it leaves us with the original bug to fix. Date PopUp can now be used again with Storm (as Ticket and Invoice fields are not using this module).

I'll look at this again.

One problem is that I don't think the Date Popup module allows for a NULL/0 value to be selected for a date.
For example when you use Date PopUp for the payment date, if there is no payment date you can't remove the date.

This could be an issue for the Date module too and also a reason why is was not implemented on Tickets and Invoices in the first place.

Because there wasn't any activity since February .
New attempt to fix this.

Instead of check every time if the module exists and change the values for date popup and change it back, so the rest works, in every form, i decided to try to do this check in the dateext field directly.
And it worked!
And date_popup allows empty dates, i added a check if withnull is true, other wise i do not allow empty dates.

So all the checks happens now in storm.module in the dateext form. We use this already everywhere.
I added a option in our form element to disable the date_popup, but i don't know where we should need this ;)
If there is no date given, the field is blank. And the date_popup calendar now uses the defined year range set in the storm settings.

So here the patch for it, what do the other think about this method?

#36: date_popup_integration-787822-36.patch queued for re-testing.

I haven't tested this - but read it through. Looks like a really neat way of solving this - makes putting a date field in a lot simpler each time.

+++ b/storm.module
@@ -423,46 +421,111 @@ function storm_dateext_expand($element) {
+    //TODO should we make date_format a option?

What option would this add that we don't already have?

Uh this comment is on a misleading postion...
I meant this line:
'#date_format' => "Y-m-d",

In all types date_popup is implemented, it uses "Y-m-d" as format string, when submitting the form. So if you select a date with the calendar, then the javascript writes in the input field "2011-09-29".
It can be changed, so maybe it would be nice to add an option for this.
But then i don't know if strtotime converts this string properly to a timestamp. So this is i only a notice and I forgot to look it up and remove it.
According to the php manual, strtotime works best with Y-m-d and for all other formats DateTime::createFromFormat should be used, with comes with php5.3.
So i think we shouldn't make this an option and let it Y-m-d.

Sounds like good reasoning.

I've rerolled - the only change is the removal of the comment, since it seems not to be required.

Pushed the Patch into the 6.x-2.x-dev Branch.

Rerolled Patch against 6.x-1.x-dev

Should this go into 1.x?

I'm trying to keep 1.x as bug fixes + tests only.

Feel free to try to persuade me otherwise...

/me falls on his knees and with folded hands pleads.... please oh pretteh please... please commit this one to 1.x

The rationale behind "bug fixes only" for 1.x is that it makes the branch more stable for those users who "have what they need" (and means that we draw a line so that time can be spent on 2.x).

I assume you can't upgrade to 2.x at the moment?

If not, could you please test the patch for 1.x and report back? Perhaps we could commit it if it has had several positive reviews (patches in the Storm queue seem to rarely get reviews from others).

#44: date_popup_integration-787822-44.patch queued for re-testing.

Please dont commit, this to 1x. Even thought i might appear to work, it doesn't. ( as i mentioned in this comment )

Restating.

the date popup appears as expected... but on each node is saved, the date is getting shifted by "MINUS 1 Day".

Test Procedure:

1> Create a storm invoice node ( after creating a organization). My timezone is configured for +5:30 GMT, ( IST, asia, kolkata).

2> Used date popup to fill 1st Dec 2011 in the date field. Then i used devel module to check the details about node load. It showed 324665000 against the respective datefield ( 324665000 when converted to date gives Friday, December 23rd 2011, 18:30:00 (GMT).)

I have not tested for similar misbehavior for 2.x yet.

The funny part is. if i keep on saving node without altering any values on the node_form ... both the date fields keeps on decreasing by one day.
------------------------------------------------------------------------------------
EDIT NOTE: Edited for clarity.

#49 holds true for 2.x.

wow, big thanks for this report!
The problem is the timezone. The Problem should only occur, when GMT+0 date is another than yours. For you this is a span of 5,5h hours, so easier to spot for you, than for me, because its only 1h for me to spot it.
Changed my timezone on my PC to check this.

Here the patch for 2.x Branch.
I changed from strtotime to preg_match, because the format is hardcoded.

okay, worked for 2.x. commited this.

reroll against 6.x-1.x.

Please review and test. We could commit it if it has had several positive reviews.

sorry it will be few days before i could test this :(. I fried my third hard disk in a month forcing me to believe there is some issue with the mother board.

I will check this out as soon as my set up is ready and working. Unfortunately i have lost few work of mine too.

@wow, big thanks for this report! - kfritsche, i am really pleased to be useful. But compared to the mammoth effort you and your team is putting in... i feel humble ( not at all a character of mine :P).

Hi there, did a quick install on my server and checked it out.

The issue still exist. its not removed in 2x branch.

The attached screen shots was prepared by using edit and save buttons to jumb from one state to other. NO MANIPULATION of any other form fields were done manually.

As you can see above in #54, there seems to be problem when transiting from 002 to 003.... that is when somebody is going to edit the invoice, and the date is loaded imporperly.

But lets have a look at what devel has to say.

state 001:
On node view we see this date against Request date.
Request date: 2011-12-01

But when inspecting with devel the node has stored the value.
requestdate (String, 10 characters ) 1322677800.

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Analysis.
what does this cryptic unix time stamp translate to...
1322677800 is Wed, 30 Nov 2011 18:30:00 GMT by using this website

on my system,
dsm (date('Y-m-d',1322677800)) ------------> 2011-11-30

dsm (date('D M j G:i:s T Y',1322677800)) ----> Wed Nov 30 18:30:00 UTC 2011

So it begs me a question, why does 002.png seems to show proper date.
and the answer is, it uses a theme function which respects user's time zone.
"format_date($node->requestdate, 'custom', 'Y-m-d')".

dsm ( format_date(1322677800, 'custom', 'D M j G:i:s T Y')) -----> Thu Dec 1 0:00:00 GMT 2011
or just the Y-m-d part...
dsm ( format_date(1322677800, 'custom', 'Y-m-d')) ---> 2011-12-01.

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Conclusion.
CONDITION if TIMEZONE is Positive.
The user time zone is GMT +0530 hours.

User click add/edit invoice.
Let say the user enters Request date:
2011-12-01
This value is treated as 2011-12-01 00:00 by the system.

The date is getting stored as "User entered Time" Minus "User's TimeZone".
ie. 2011-12-01 00:00:00 - 05:30:00 = 2011-11-30 18:30:00

So next time he/she visits the node edit form, the user is presented with last entered value Minus Timezone.
2011-11-30 18:30:00, But not exactly the time part is stripped off from the field.
so the user is presented with actually the following date.
2011-11-30 00:00:00

Now you can guess what would happen if the user clicks the save button.

The date will stored as 2011-11-29 16:30:00

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Possible work around.
Seems like when we are displaying a date and accepting the date, the user time zone has to be respected, but when we are storing the date, we will have to store it using unix time stamp.

Damn me, attached the wrong patch, as you can see on the naming, its the same as attached to comment 44.

So here the real thing, which should fix exactly this problem.

Is the patch in #56 for 2.x or 1.x?

if its for 2.x then it fails to apply on my server..

$ git status
# On branch 6.x-2.x
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       date_popup_integration-787822-51.patch
nothing added to commit but untracked files present (use "git add" to track)
$ git apply -v date_popup_integration-787822-51.patch
Checking patch storm.css...
Checking patch storm.module...
error: while searching for:
    '#input' => TRUE,
    '#process' => array('storm_dateext_expand'),
    '#element_validate' => array('storm_dateext_validate'),
    '#default_value' => array(
      'day' => format_date(time(), 'custom', 'j'),
      'month' => format_date(time(), 'custom', 'n'),
      'year' => format_date(time(), 'custom', 'Y'),
    ),
  );
  return $type;
}

error: patch failed: storm.module:342
error: storm.module: patch does not apply
Checking patch stormexpense/stormexpense.module...
error: while searching for:
  );

  $form['group2']['expensedate'] = array(
    '#type' => 'date',
    '#title' => t('Date'),
    '#default_value' => _storm_gmtimestamp_to_date($node->expensedate),
  );

  $form['group2']['provider_title'] = array(

error: patch failed: stormexpense/stormexpense.module:432
error: stormexpense/stormexpense.module: patch does not apply
Checking patch storminvoice/storminvoice.module...
error: while searching for:
    '#type' => 'dateext',
    '#withnull' => 'true',
    '#title' => t('Request date'),
    '#default_value' => _storm_gmtimestamp_to_date($node->requestdate),
  );

  $form['group3']['duedate'] = array(
    '#type' => 'dateext',
    '#withnull' => 'true',
    '#title' => t('Due date'),
    '#default_value' => _storm_gmtimestamp_to_date($node->duedate),
  );

  $form['group3']['paymentdate'] = array(
    '#type' => 'dateext',
    '#withnull' => 'true',
    '#title' => t('Payment date'),
    '#default_value' => _storm_gmtimestamp_to_date($node->paymentdate),
  );

  $form['group4'] = array(

error: patch failed: storminvoice/storminvoice.module:432
error: storminvoice/storminvoice.module: patch does not apply
Checking patch stormproject/stormproject.module...
error: while searching for:
    '#type' => 'dateext',
    '#title' => t('Date begin'),
    '#withnull' => true,
    '#default_value' => _storm_gmtimestamp_to_date($node->datebegin),
  );

  if (module_exists('date_api') && module_exists('date_popup')) {
    $form['group3']['datebegin'] = array(
      '#type' => 'date_popup',
      '#title' => t('Date begin'),
      '#date_format' =>  'Y-m-d',
      '#default_value' => date('Y-m-d',$node->datebegin),
    );
  }

  $form['group3']['dateend'] = array(
    '#type' => 'dateext',
    '#title' => t('Date end'),
    '#withnull' => true,
    '#default_value' => _storm_gmtimestamp_to_date($node->dateend),
  );

  if (module_exists('date_api') && module_exists('date_popup')) {
    $form['group3']['dateend'] = array(
      '#type' => 'date_popup',
      '#title' => t('Date end'),
      '#date_format' =>  'Y-m-d',
      '#default_value' => date('Y-m-d',$node->dateend),
    );
  }

  $durationunit_list = stormattribute_attributes_bydomain('Duration unit');
  $form['group3']['durationunit'] = array(
    '#type' => 'select',
    '#title' => t('Duration unit'),

error: patch failed: stormproject/stormproject.module:421
error: stormproject/stormproject.module: patch does not apply
Checking patch stormtask/stormtask.module...
error: while searching for:
    '#type' => 'dateext',
    '#title' => t('Date begin'),
    '#withnull' => true,
    '#default_value' => _storm_gmtimestamp_to_date($node->datebegin),
  );

  if (module_exists('date_api') && module_exists('date_popup')) {
    $form['group3']['datebegin'] = array(
      '#type' => 'date_popup',
      '#title' => t('Date begin'),
      '#date_format' =>  'Y-m-d',
      '#default_value' => date('Y-m-d',$node->datebegin),
    );
  }

  $form['group3']['dateend'] = array(
    '#type' => 'dateext',
    '#title' => t('Date end'),
    '#withnull' => true,
    '#default_value' => _storm_gmtimestamp_to_date($node->dateend),
  );

  if (module_exists('date_api') && module_exists('date_popup')) {
    $form['group3']['dateend'] = array(
      '#type' => 'date_popup',
      '#title' => t('Date end'),
      '#date_format' =>  'Y-m-d',
      '#default_value' => date('Y-m-d',$node->dateend),
    );
  }

  $durationunit_list = stormattribute_attributes_bydomain('Duration unit');
  $form['group3']['durationunit'] = array(
    '#type' => 'select',

error: patch failed: stormtask/stormtask.module:602
error: stormtask/stormtask.module: patch does not apply
Checking patch stormticket/stormticket.module...
error: while searching for:
    '#type' => 'dateext',
    '#title' => t('Date begin'),
    '#withnull' => TRUE,
    '#default_value' => _storm_gmtimestamp_to_date($node->datebegin),
  );

  $form['group3']['dateend'] = array(
    '#type' => 'dateext',
    '#title' => t('Date end'),
    '#withnull' => TRUE,
    '#default_value' => isset($node->dateend) ? _storm_gmtimestamp_to_date($node
->dateend) : NULL,
  );

  $durationunits = stormattribute_attributes_bydomain('Duration unit');

error: patch failed: stormticket/stormticket.module:491
error: stormticket/stormticket.module: patch does not apply
Checking patch stormtimetracking/stormtimetracking.module...
error: while searching for:
  $form['group2']['trackingdate'] = array(
    '#type' => 'dateext',
    '#title' => t('Date'),
    '#default_value' => _storm_gmtimestamp_to_date($node->trackingdate),
  );

  $form['group3'] = array(

error: patch failed: stormtimetracking/stormtimetracking.module:438
error: stormtimetracking/stormtimetracking.module: patch does not apply

It is for 6.x-1.x - its Patch #43 and #51 merged into one and downgraded for 6.x-1.x

Hi Karl, i dont mean to ruin your weekend, but see i applied the patch successfully on 1.x. But the problem is not solved. The behavior is same.

And the next patch...
I used every where format_date, except on 2 places. Fixed this.

This is the Patch for 6.x-2.x!

And here the full patch for 6.x-1.x

#60 Works and fixes the issue for 6.x2.x-dev.

#62 Works and fixes the issue for 6.x-1.x

Patch for 6.x-2.x is commited.

@juliangb: What do you think about the backport to 6.x-1.x? D34dMan and me tested it. Its sort of new feature and we said, to not add new features to 6.x-1.x, but this would fix #1178598: Date begin and Date end are reset to 1970-01-01 and maybe even #377680: Support for node preview.

If it fixes bugs, then yes, feel free.

The reason for only committing bug fixes is for stability - but its ok if a couple of people have tested it (doesn't always happen).

commited to 6.x-1.x

Found a minor error with one-digit dates like 1-1-2012 and datepopup.

Will commit this patch, after testing got through.

Commited to 6.x-1.x and 6.x-2.x.