Problem/Motivation

The magic code used to determine whether a date is "all day" has caused major problems.

Proposed resolution

Magic code needs to be removed in favor of a simple value stored in the database to indicate a date is considered "all day".

Remaining tasks

  • Add a new column to the module's base field definition.
  • Rework the all-day logic to use the new field.
  • Add test coverage for the update script.
  • Fix the "text field" widget when using a "Date" field.
  • Update the new update script to verify the columns were actually removed during the prep work.
  • Code cleanup.
  • Test coverage for all all-day field types.
  • More test coverage.
  • Regression testing, including testing other modules.
  • Write a change notice.
  • Update scripts for converting existing data go into #3201878.
  • Anything else?

User interface changes

None.

API changes

TBD.

Data model changes

TBD.

Comments

DamienMcKenna created an issue. See original summary.

damienmckenna’s picture

Priority: Major » Critical
Parent issue: » #3201850: Plan for Date 7.x-2.12-rc1
sgdev’s picture

I agree. One thing to consider is the way an "All day" value is stored includes a full datestamp, but it essentially ignores the time portion:

Stored Start: 2020-01-01 00:00:00 --> 2020-01-01
Stored End: 2020-01-01 00:00:00 --> 2020-01-01 (would expect 2020-01-01 23:59:59, but module "chops off" time)

So in that sense, it is a rather "simple" value, but one that attempts to repurpose a full datestamp into a value that only uses the day.

Whatever the approach, it will need to take into account that date fields can be configured in the interface to *not* store seconds, minutes, and even hours (always store as 00). In our implementations, users only enter hours and minutes with a timepicker, and are restricted to only be able to select :00, :15, :30, and :45.

damienmckenna’s picture

Status: Active » Needs review
StatusFileSize
new2.24 KB

WIP patch to add the necessary field schema changes.

damienmckenna’s picture

Status: Needs review » Needs work

This needs further work to update the internal APIs, and then fix existing data.

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new6.69 KB

A test for the update script. Note that this kinda cheats because the field is not fully removed before the update script attempts to add it back again. Also, none of the internal APIs have been updated yet.

damienmckenna’s picture

Assigned: Unassigned » damienmckenna
damienmckenna’s picture

Status: Needs review » Needs work
damienmckenna’s picture

StatusFileSize
new7.13 KB
new11.98 KB

WIP, didn't want to loose it.

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new7.94 KB
new18.66 KB

WIP to use hook_field_widget_form_alter() to add the all-day option.

Status: Needs review » Needs work

The last submitted patch, 10: date-n3202962-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damienmckenna’s picture

StatusFileSize
new3.68 KB
new19.3 KB

Further WIP.

damienmckenna’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: date-n3202962-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

steinmb’s picture

Damien
I just wanted to say, thank you for working on this, and all you hard work.

mustanggb’s picture

Big thanks from me too, started to look at 2.11 for the PHP 7.3/7.4 fixes, but should probably hold off until 2.12 lands.

By the way I know there is a warning in the release notes: https://www.drupal.org/project/date/releases/7.x-2.11

But it's probably worth adding one at the top of the project page as well: https://www.drupal.org/project/date

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new3.32 KB
new21.29 KB

Further work.

damienmckenna’s picture

I realized that now we have a specific value for indicating "this is an all-day thing" we don't need most of the code in theme_date_all_day().

Status: Needs review » Needs work

The last submitted patch, 17: date-n3202962-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damienmckenna’s picture

The one code test works, now to fix the update script, clean up the code, write more tests, and look at backwards compatibility. Little things ;-)

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new3.34 KB
new21.47 KB

I realized it was looking for the wrong field name - the suffix is "all_day" rather than "is_all_day". It also didn't need to fully clear the caches, just rebuild the schema.

damienmckenna’s picture

Issue summary: View changes
Status: Needs review » Needs work

Tests are green! Woot!
Next off:

  • Update the new update script to verify the columns were actually removed during the prep work.
  • Code cleanup.
  • More test coverage.
  • Regression testing, including testing other modules.
  • Write a change notice.
  • Update the issue summary.
  • Anything else?
damienmckenna’s picture

Issue summary: View changes

Spotted another bug - when you try to set a date to "all day" and it is using the "text field" widget it gives an error if the value includes a time portion, e.g. "07/22/2021 - 17:30" might be the default value from the widget but it fails, it actually requires the value "07/22/2021" in order to pass validation.

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new26.11 KB
new6.98 KB

This includes the following changes:

  • Test coverage for the "Text field" widget, that fails.
  • Cleaned up date_all_day_field() to remove unused code.
  • Updated date_field_all_day() to match date_all_day_field().

I know the first one will fail, hopefully the others are ok.

Status: Needs review » Needs work

The last submitted patch, 24: date-n3202962-24.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new6.21 KB
new29.54 KB

The field validation needed to be changed for the all_day validation to work. That one test works correctly, let's test the whole thing.

damienmckenna’s picture

StatusFileSize
new36.95 KB
new17.09 KB

Refactoring the tests to rep for more test coverage.

damienmckenna’s picture

StatusFileSize
new9.44 KB
new40.46 KB

New content type.

damienmckenna’s picture

StatusFileSize
new45.06 KB
new18.3 KB

More test coverage.

damienmckenna’s picture

Issue summary: View changes
Status: Needs review » Needs work

So that fixes the text field widget.

Now to add test coverage for the timestamp field and its widgets.

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new53.54 KB
new12.12 KB

Test coverage for the timestamp field.

Status: Needs review » Needs work

The last submitted patch, 31: date-n3202962-31.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damienmckenna’s picture

Status: Needs work » Needs review
StatusFileSize
new1018 bytes
new53.54 KB

Doh. Typoo.

damienmckenna’s picture

Do we need more test coverage, especially unit test coverage, of the all-day functionality? Any thoughts?

damienmckenna’s picture

Issue summary: View changes
Status: Needs review » Needs work

FYI #3201878 will focus on updating existing data to the new system.

joseph.olstad’s picture

So to test this, what is the test plan and what to look for ?

install from fresh vanilla, set up all day value
compare timezone settings and stored results in the db tables ? and also the ui, test dates in views also?

I have a 7.80 environment I could run this on.

probably will have a closer look at the automated tests, could use that as a test plan.

damienmckenna’s picture

We need to identify whether other modules break because of the change. We also need to test it with e.g. timezone changes.

damienmckenna’s picture

I also wonder if this should require a new branch, i.e. to take over the 7.x-3.x branch?

joseph.olstad’s picture

ya I would be a proponant of taking over the 7.x-3.x branch and cutting a release, allow a few people to try it in the wild, then if no major issues, set it as the recommended release branch as 7.x-3.0

damienmckenna’s picture

I created a draft change notice.

joseph.olstad’s picture

change request looks good thanks @DamienMcKenna

damienmckenna’s picture

Issue summary: View changes
Status: Needs work » Fixed

Will do new issues for some of the pieces.

I've committed this as-is to the 7.x-3.x branch.

damienmckenna’s picture

Assigned: damienmckenna » Unassigned
damienmckenna’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev

  • DamienMcKenna committed efbbe9a on 7.x-3.x
    Issue #3202962 by DamienMcKenna, ron_s, MustangGB: Expand field...

Status: Fixed » Closed (fixed)

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

damienmckenna’s picture

lwalley’s picture

For reference, there is a remaining UX issue when editing date popup with time field with all day checked on 7.x-3.x branch, see #3274909: Error when time is missing from an all-day event, in summary UX issue is:

User checks All Day which hides time field via JS, then on node save an error occurs saying time field is required.

Workaround: user must uncheck All Day to show time field, add time e.g. 00:00 then re-check All Day and then save will succeed without error.