Fixed Logbook Card/Panel/Dialog Incorrect Entires for `input_datetime` Entities #9399

ShaneQi · 2021-06-10T01:23:36Z

Proposed change

The issue (in the video, although I changed the input_datetime to different dates, logbook dialog's every entry shows the same date):

Kapture.2021-06-09.at.20.12.20.mp4

The issue under the hood is:
Back in #6978 we added a parameter state to computeStateDisplay in order to compute the display of an explicit state (instead of the entity's current state). But the input_datetime handling wasn't changed properly.
The input_datetime handling should try to format the passed in explicit state if it exists.
This PR added the handling for passed in explicit input_datetime state.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

N/A

Additional information

This PR fixes or closes issue: fixes Logbook Card/Panel/Dialog Incorrect Entires Texts for input_datetime Entities #9405
This PR is related to issue or discussion: Logbook: Localize Message & Style updates #6978
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

…n explicit input_datetime state.

…input_datetime state display.

ShaneQi · 2021-06-12T20:18:33Z

src/common/entity/compute_state_display.ts

+          // When only date is passed to `Date` constructor, it uses UTC regardless of frontend's timezone,
+          // so we need to add the frontend's timezone offset.
+          // Other usages of `Date` constructor have both date and time in the string, frontend's timezone is used w/o problem.
+          const timezoneOffset = new Date().getTimezoneOffset() * 60 * 1000;
+          const adjustedDateObj = new Date(dateObj.getTime() + timezoneOffset);


Safari does not accept this format for the Date constructor at all, it will return Invalid Date

bramkragten · 2021-06-15T13:49:53Z

So the issue here is that we don't have the attributes in the logbook?

src/common/entity/compute_state_display.ts

bramkragten · 2021-06-15T13:57:11Z

src/common/entity/compute_state_display.ts

+        const now = new Date();
+        date = new Date(


Instead of creating a new date, we can just set the hour and minute of now?

This is done.

bramkragten · 2021-06-15T13:59:08Z

src/common/entity/compute_state_display.ts

+      // If trying to display an explicit state, need to parse the explict state to `Date` then format.
+      // Attributes aren't available, we have to use `state`.
+      try {
+        if (!stateObj.attributes.has_time) {


If attributes are not available, how can we do this?

bramkragten · 2021-06-15T14:06:58Z

src/common/entity/compute_state_display.ts

+      try {
+        if (!stateObj.attributes.has_time) {
+          // Only has date.
+          const dateObj = new Date(state);


I think we should make the state string a valid ISO 8601 string, by adding a date or time, then we don't have to struggle with all these issues, and don't have to rely on the current attributes.

ShaneQi · 2021-06-15T21:03:37Z

@bramkragten thanks for taking a look at this PR!

So the issue here is that we don't have the attributes in the logbook?

Yes, we only added icon and state for logbook (back in home-assistant/core#40039), no attributes are sent in the API.

If attributes are not available, how can we do this?

We are getting has_time and has_date from the entity's current attributes.
Because we don't have the event's attributes, I was making the assumption that the entity's has_time and has_date attributes didn't change. That can be wrong, but very rarely to be wrong IMO.
I hope we end up with a solution in which we don't need to make this assumption.

Instead of creating a new date, we can just set the hour and minute of now?

This code exists prior to this PR, I only changed indentation. But yes, I think we can, I can try it.

Safari does not accept this format for the Date constructor at all, it will return Invalid Date

The Date constructor doesnt throw on an invalid date.

I think we should make the state string a valid ISO 8601 string, by adding a date or time, then we don't have to struggle with all these issues, and don't have to rely on the current attributes.

To the 3 comments above, I think you are making good points. There seems to be too many issues and hacks when we try to create a Date object from state.

2 questions on this direction:

For making state a ISO 8601 string

Do we make core store the input_datetime's state in ISO 8601? (This sounds to be too big of a breaking change to me)

Or we add logic to core logbook component to process input_datetimes' states into ISO 8601?

Or do we do it on frontend? We would do something like these?:

date and time: 2021-06-03 01:06:00 -> 2021-06-03T01:06:00Z
// split by space, join with `T`, then append `Z`

time only: 16:11:00 -> 2021-06-15T16:11:00Z
// today's ISO 8601 date string + `T` + state + `Z`

date only: 2021-08-07
// this is already ISO 8601

// I think all 3 above need to add timezone offset
// because they are all UTC while the `state` string was generated in the user's timezone

For `has_time` and `has_date` attributes

If we don't want to rely on the current attributes, I think the only way is the make core logbook component return them, right?

Or are you thinking about 'looking at state to figure out if it is date or time or both'?

bramkragten · 2021-06-15T21:21:16Z

Yes, I think the frontend should create a ISO 8601 string.

We don't need to use the has_time and has_date attributes, it can be derived from the state anyway? But I agree that it is an edge case that the current state would have different settings than the historic states. But if we are going to create an ISO string, we need to make sure the state matches those props anyway, otherwise we will get a ton of errors.

About timezone:
If there is a time in an ISO 8601 string, it won't be treated as UTC, just when it is just a date it would be UTC.

ShaneQi · 2021-06-15T22:44:21Z

@bramkragten gotcha, thanks for the advice! I will go with this direction see how it works out.

For timezone: doesn't the letter Z mean UTC? e.g. 2021-06-15T16:11:00Z meaning 16:11:00 UTC (but the state from core 16:11:00 meant to say it's 16:11:00 in the user's timezone therefore we need to add timezone offset).
I might be confused tho right now, will figure it out when I actually debug it.

Thanks again.

ShaneQi · 2021-06-16T20:10:39Z

@bramkragten Finished refactoring to what we discussed.

Let me know if I've misunderstood anything or anything can be improved.
There are lots of ways to convert the state string to IS8601 as well as derive date only/time only/datetime from it. I'm open to any suggestions of changes.

bramkragten · 2021-06-17T20:05:29Z

src/common/entity/compute_state_display.ts

+        const components = state.split(" ");
+        if (components.length === 2) {
+          // Date and time.
+          const iso8601String = components.join("T") + "Z";


If you leave the Z of, you don't need the timezoneOffset

I thought Z is required in ISO8601 spec, even after reading some documents, it's still vague.

But I verified it works without Z on both Chrome and Safari, I will remove it. Thanks for the reminder!

bramkragten · 2021-06-17T20:06:31Z

src/common/entity/compute_state_display.ts

+            // Inserting today's Date string.
+            const now = new Date();
+            const dateISO8601String =
+              now.toISOString().substring(0, 10) + "T" + state + "Z";


Same here, no need for timezoneOffset when Z is dropped

bramkragten · 2021-06-17T20:09:03Z

src/common/entity/compute_state_display.ts

+            const dateObj = new Date(state);
+            const adjustedDateObj = new Date(
+              dateObj.getTime() + timezoneOffset
+            );
+            return formatDate(adjustedDateObj, locale);


This will work, but

const dateObj = new Date(`${state}T00:00:00`);

will work too.

bramkragten · 2021-06-17T20:17:41Z

src/common/entity/compute_state_display.ts

+            // Inserting today's Date string.
+            const now = new Date();
+            const dateISO8601String =
+              now.toISOString().substring(0, 10) + "T" + state + "Z";


Suggested change

now.toISOString().substring(0, 10) + "T" + state + "Z";

`${now.toISOString().split("T")[0]}T${state};

ShaneQi · 2021-06-18T03:37:07Z

@bramkragten I resolved the last 4 comments. I'm amazed how simple this ended up with, thank you!

src/common/entity/compute_state_display.ts

Co-authored-by: Bram Kragten <[email protected]>

Fixed the issue that computeStateDisplay doesn't treat the passed i…

aa5cf27

…n explicit input_datetime state.

homeassistant added the cla-signed label Jun 10, 2021

ShaneQi marked this pull request as draft June 10, 2021 01:24

ShaneQi mentioned this pull request Jun 10, 2021

Logbook Card/Panel/Dialog Incorrect Entires Texts for input_datetime Entities #9405

Closed

3 tasks

ShaneQi added 3 commits June 12, 2021 19:32

Assuming timezone is same on core's config and browser for computing …

f631692

…input_datetime state display.

Added comments on timezone issue.

83127a0

Updated comments.

4842cfe

ShaneQi commented Jun 12, 2021

View reviewed changes

ShaneQi marked this pull request as ready for review June 12, 2021 20:18

ShaneQi added 2 commits June 13, 2021 20:37

Added tests for computing explicit state in computeStateDisplay.

f4fd749

Removed confusing comment to avoid over-complicate things.

d81eb33

bramkragten reviewed Jun 15, 2021

View reviewed changes

src/common/entity/compute_state_display.ts Show resolved Hide resolved

bramkragten reviewed Jun 15, 2021

View reviewed changes

ShaneQi added 3 commits June 16, 2021 19:41

Removed confusing comment to avoid over-complicate things.

0854062

Simplified a state computation.

6dab131

Added comments.

2493f8c

bramkragten reviewed Jun 17, 2021

View reviewed changes

ShaneQi added 2 commits June 18, 2021 02:49

Simpified state to date convertion.

2b0f19b

Fixed a typo.

5c6f96a

bramkragten reviewed Jun 29, 2021

View reviewed changes

src/common/entity/compute_state_display.ts Outdated Show resolved Hide resolved

bramkragten reviewed Jun 29, 2021

View reviewed changes

src/common/entity/compute_state_display.ts Outdated Show resolved Hide resolved

bramkragten reviewed Jun 29, 2021

View reviewed changes

src/common/entity/compute_state_display.ts Outdated Show resolved Hide resolved

bramkragten approved these changes Jun 29, 2021

View reviewed changes

ShaneQi and others added 2 commits June 29, 2021 16:16

Apply suggestions from code review

75b8645

Co-authored-by: Bram Kragten <[email protected]>

Fixed lint issue.

e76bbd1

bramkragten merged commit db37dff into home-assistant:dev Jun 29, 2021

bramkragten mentioned this pull request Jun 30, 2021

20210630.0 #9483

Merged

github-actions bot locked and limited conversation to collaborators Jun 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Logbook Card/Panel/Dialog Incorrect Entires for `input_datetime` Entities #9399

Fixed Logbook Card/Panel/Dialog Incorrect Entires for `input_datetime` Entities #9399

ShaneQi commented Jun 10, 2021 •

edited

Loading

ShaneQi Jun 12, 2021

bramkragten Jun 15, 2021 •

edited

Loading

bramkragten commented Jun 15, 2021

bramkragten Jun 15, 2021

ShaneQi Jun 16, 2021

bramkragten Jun 15, 2021

bramkragten Jun 15, 2021

ShaneQi commented Jun 15, 2021 •

edited

Loading

bramkragten commented Jun 15, 2021 •

edited

Loading

ShaneQi commented Jun 15, 2021 •

edited

Loading

ShaneQi commented Jun 16, 2021 •

edited

Loading

bramkragten Jun 17, 2021

ShaneQi Jun 18, 2021

bramkragten Jun 17, 2021

bramkragten Jun 17, 2021 •

edited

Loading

bramkragten Jun 17, 2021

ShaneQi commented Jun 18, 2021

	now.toISOString().substring(0, 10) + "T" + state + "Z";
	`${now.toISOString().split("T")[0]}T${state};

Fixed Logbook Card/Panel/Dialog Incorrect Entires for input_datetime Entities #9399

Fixed Logbook Card/Panel/Dialog Incorrect Entires for input_datetime Entities #9399

Conversation

ShaneQi commented Jun 10, 2021 • edited Loading

Proposed change

Type of change

Example configuration

Additional information

Checklist

Choose a reason for hiding this comment

bramkragten Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

bramkragten commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneQi commented Jun 15, 2021 • edited Loading

For making state a ISO 8601 string

For has_time and has_date attributes

bramkragten commented Jun 15, 2021 • edited Loading

ShaneQi commented Jun 15, 2021 • edited Loading

ShaneQi commented Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bramkragten Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneQi commented Jun 18, 2021

Fixed Logbook Card/Panel/Dialog Incorrect Entires for `input_datetime` Entities #9399

Fixed Logbook Card/Panel/Dialog Incorrect Entires for `input_datetime` Entities #9399

ShaneQi commented Jun 10, 2021 •

edited

Loading

bramkragten Jun 15, 2021 •

edited

Loading

ShaneQi commented Jun 15, 2021 •

edited

Loading

For `has_time` and `has_date` attributes

bramkragten commented Jun 15, 2021 •

edited

Loading

ShaneQi commented Jun 15, 2021 •

edited

Loading

ShaneQi commented Jun 16, 2021 •

edited

Loading

bramkragten Jun 17, 2021 •

edited

Loading