Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to specify format and parsing format in dateHelper functions #13500

Merged

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Nov 29, 2022

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #13477

Description

Currently when using dateHelper.getLocalDate() this expect to parse date in format YYYY-MM-DD HH:mm:ss, but sometimes the date may be different, e.g. returned from "Label (datetime)" when fetching contentResource.getById(nodeId) and looping its properties. Depending on the backoffice user the value property may be different, see #13477 (comment)

Furthermore moment.js also allow to use moment.ISO_8601 here when parsing from a known date format: https://momentjs.com/guides/#/parsing/known-formats/

Start Date property is using Date Picker and End Date was using Date Picker, but switched property editor to Label (datetime) after selecting a date. In both cases date is December 1st 2022.

image

image

In the following example from a dashboard formatDatesToLocal() function d3 is returning an incorrect date, where currentUser.locale is en-US. On the other hand d1 and d2 returns the same correct parsed date.

I have attached the dashboard here - just unzip it to App_Plugins folder 🤗
MyDashboard.zip
Furthermore with default starterkit add a "Details" group to Home Page document type with two properties "Start Date (startDate)" and "End Date (endDate)" using standard "Date Picker" datatype instance with default configuration. Next go to Home Page node and select December 1st 2022 in both date pickers and click Save & Publish. Go back to Home Page document type and change End Date property to use standard "Label (datetime)" datatype instead.
You probably also need to change value of id property in showDetails function in dashboard.controller.js to match the node id of Home Page.

Now inspect the data in console when clicking "Show details" button in dashboard.

It is possible to handle this using moment.js directly or add your own service, but it would be great to use core dateHelper for this, which may also solve issues if developers change default from in date pickers.

function formatDatesToLocal(prop, format) {

  format = format || "LLL";

  // get current backoffice user and format dates
  userService.getCurrentUser().then(currentUser => {
    console.log("locale", currentUser.locale);

    console.log("value", prop.value);

    const parsingFormat = "MM/DD/YYYY HH:mm:ss";

    const d1 = moment(prop.value, parsingFormat).locale(currentUser.locale).format(format);
    const d2 = dateHelper.getLocalDate(prop.value, currentUser.locale, format, parsingFormat);
    const d3 = dateHelper.getLocalDate(prop.value, currentUser.locale, format);

    console.log("d1", d1);
    console.log("d2", d2);
    console.log("d3", d3);

    //prop.value = moment(prop.value, 'MM/DD/YYYY HH:mm:ss').locale(currentUser.locale).format(format);
    //prop.value = dateHelper.getLocalDate(prop.value, currentUser.locale, format);
  });
}
chrome_JJQxS2aJ9b.mp4

switching language of backoffice user to Danish still seems to work although the date format is not MM/DD/YYYY HH:mm:ss but instead DD.MM.YYYY HH:mm:ss

image

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 29, 2022

Okay in a working project I had to use this to switch parsing format based on user locale:

function formatDatesToLocal(prop, format) {

  format = format || "LLL";

  // get current backoffice user and format dates
  userService.getCurrentUser().then(currentUser => {
      const culture = currentUser.locale;
      let parsingFormat = "MM/DD/YYYY HH:mm:ss";

      if (culture === "da-DK")
      {
          parsingFormat = "DD.MM.YYYY HH:mm:ss";
      }

      prop.value = moment(prop.value, parsingFormat).locale(culture).format(format);
      //prop.value = dateHelper.getLocalDate(prop.value, currentUser.locale, format);
      
      // This doesn't exist in core yet.
      //prop.value = dateHelper.getLocalDate(prop.value, currentUser.locale, format, parsingFormat);
  });
}

I am not sure if there is a specific way label property editor format the date value based on user locale, so we know how to parse the format a bit more dynamically?

@nul800sebastiaan nul800sebastiaan changed the base branch from v11/contrib to contrib January 26, 2023 14:47
@emmagarland
Copy link
Contributor

Hi @bjarnef,

Thanks for your detailed description, dashboard and replication steps!

Before I start looking at getting this one running, as the history of this is quite long, please could you give me a summary of:

  • What questions you need answering from HQ (if any?)
  • What scenario the PR is resolving
  • If the PR is ready for review?

Once I have that, I will know how to test the current behaviour, and then observe the change that the PR makes, and we can progress from there.

Best,

Emma 😃

@bjarnef
Copy link
Contributor Author

bjarnef commented May 21, 2023

Hi @emmagarland

The use-case it that we are storing some datetime properties in "Label (datetime)" on some course (content) nodes.
Instead of the default listview in a content app, we have removed this and added our own view in a content app with advanced searching, filtering and sorting on these course node node.

Some of the node properties are shown in table rows (custom directive) and furthermore we have for each row an inline button to show more details. The details view/overlay get the course nodes data using contentResource.getById(nodeId) to get the course properties, but filtered the ones we want to display. However from the API the data in the property value is different format depending on backoffice user language. #13477 (comment)

So the parsing would fails if date time (from value property) isn't in format YYYY-MM-DD HH:mm:ss
And in details overlay we use something like this to list the properties, but manipulation date in value property for each datetime specific property shown in the details overlay as we didn't need the timestamp:

<umb-property
    ng-repeat="property in vm.properties track by property.alias"
    property="property">
    <umb-property-editor model="property"></umb-property-editor>
</umb-property>

Content node
image

Details overlay
image

So we have a workaround using moment directly.
#13500 (comment)

The changes in this PR still using YYYY-MM-DD HH:mm:ss by default as parsing format, but allow to specify a different format, e.g. when one know the date format is different. It could be when the data comes from an external source/API.

Furthermore it also allow how to format at server time format, e.g. in a dashboard. It could of course format that date once again, but seems to be redundant, if it can just be formatted correctly in first place (e.g. a server time format presented in a dashboard formatted the current backoffice user). Currently I think these methods are only used sparingly in Settings or Users section of backoffice.

and yes the PR is ready for review :)

@emmagarland emmagarland self-assigned this May 23, 2023
@emmagarland
Copy link
Contributor

Thanks @bjarnef for your reply.

I have started to try and replicate the behaviour using your provided package and instructions.

So far, I have got "Invalid date" when switching back to a label from a DateTime, but I will restart this with fresh data to double check.

image

Many thanks again, I'll be in touch soon!

Emma

@emmagarland
Copy link
Contributor

Hi @bjarnef!

I've been testing the existing behaviour using the following steps:

  1. Use a default Umbraco installation on the latest stable Umbraco version (v11.3.1) with a default locale (en-US)
  2. Install the provided dashboard package from this PR

image

  1. Create 2 datetime fields for any content item (Start Date and End Date)

image

  1. Select 1st December 2022 for both, save and publish
  2. Open the “Show Details” link on the dashboard package
  3. Observe the two dates showing as Start Date “2022-12-02 00:00:00” and End Date showing as 12/01/2022:

image

  1. Change the End Date field to be a Label (datetime)
  2. Save and publish
  3. Return to the dashboard
  4. Observe the two dates showing as Start Date “2022-12-02 00:00:00” and End Date showing as 01/20/2012:

image

  1. Switch user’s back-office language to Danish
  2. Observe the two dates showing as Start Date “2022-12-02 00:00:00” and End Date showing as 12/20/2001:

image

  1. Switch back to en-US
  2. Change the label back to a date picker
  3. Change the date to 25/05/2023
  4. Observe that the End Date now shows “Invalid Date”

Switching to your PR branch and following the same steps, step 16 still shows "Invalid Date":

image

When you get chance, please could you verify my steps above and that the same thing happens to you? Or is there a step missing in my replication steps?

Just so you know, the code in the details.controller.js in your package from line 68 is using:

 // get current backoffice user and format dates
      userService.getCurrentUser().then(currentUser => {
        console.log("locale", currentUser.locale);
        prop.value = dateHelper.getLocalDate(prop.value, currentUser.locale, format);
      });

Or is this a different and unrelated issue?

Best regards

Emma

@emmagarland
Copy link
Contributor

Hi @bjarnef

Sorry, bear with me a moment, I think I was running the wrong thing when testing this. I will let you know shortly...

Emma

@emmagarland
Copy link
Contributor

OK, using this as the correct branch (sorry!) I can confirm that now on step 16 above, using 25/05/2023, I view the following instead of "Invalid Date" on the DateTime field:

image

However, when I change to a label DateTime type, I still observe the invalid date:

image

Are you able to confirm the same, and is this something you expected would be fixed in this PR or is it something else?

Thanks 🙂
Emma

@emmagarland emmagarland added the category/dx Developer experience label Jun 8, 2023
@emmagarland
Copy link
Contributor

Hi @bjarnef

Sorry, forgot to tag you in the last comment!

I was just wondering if you were experiencing the same behaviour or if this isn't related to this PR?

Thanks

Emma

@bjarnef
Copy link
Contributor Author

bjarnef commented Jun 17, 2023

Hi @emmagarland
I need to recall how this worked, but the main idea is to be able to parse a datetime, when the stored value isn't in format YYYY-MM-DD HH:mm:ss.

E.g. if a web service/ api deliver dates in Danish format and one wanted to present the data in a dashboard.

@emmagarland
Copy link
Contributor

Thanks @bjarnef

Before merging this, I just want to check if this PR relates to #14400 I was looking at, where the Danish scheduled publish fails due to the date having full stops instead of colons.

Do you think that if PR #14400 fixes it, this would still be necessary, or could this be resolved by that PR instead?

Thanks

Emma

@emmagarland
Copy link
Contributor

@bjarnef FYI that date PR is now in contrib, so if you did have chance to pull contrib into this branch, you will be able to confirm if it is still something needed for your use cases.

😃

Emma

@nul800sebastiaan
Copy link
Member

As for #14400 - this will be a server issue and this PR will not solve it.

@emmagarland
Copy link
Contributor

Hi @bjarnef

Update on this PR.

I've managed to replicate passing a format into the getLocalDate method (after doing gulp build on Umbraco.Web.UI.Client).

Setup: As an en-GB locale user on this PR branch, I installed your dashboard package and added the properties described on your instructions, Start date and End date using standard date pickers:

image

The dashboard view:

image

The dashboard is using the code:

function formatDatesToLocal(prop, format) {

      format = format || "LLL";

      // get current backoffice user and format dates
      userService.getCurrentUser().then(currentUser => {
        console.log("locale", currentUser.locale);
        console.log("value", prop.value);

        const parsingFormat = "DD/MM/YYYY HH:mm:ss";

        const d1 = moment(prop.value, parsingFormat).locale(currentUser.locale).format(format);
        const d2 = dateHelper.getLocalDate(prop.value, currentUser.locale, format, parsingFormat);
        const d3 = dateHelper.getLocalDate(prop.value, currentUser.locale, format);

        console.log("d1 should be correct", d1);
        console.log("d2 should be correct", d2);
        console.log("d3", d3);

        //prop.value = moment(prop.value, parsingFormat).locale(currentUser.locale).format(format);
        console.log(dateHelper.getLocalDate(prop.value, currentUser.locale, format));
        prop.value = dateHelper.getLocalDate(prop.value, currentUser.locale, format, parsingFormat);
      });

    }
  1. I changed End date to a label datetime type and viewed the same properties again:

image

And the dashboard:

image

The console view:

image

This is as expected, and not passing in a format handles it as before, so it isn't breaking ("YYYY-MM-DD HH:mm:ss").

However, I am still getting the "invalid date" issue when I convert the datetime label back to a datetime picker. Are you able to replicate that it happens to you, too?

Thanks

Emma

@bjarnef
Copy link
Contributor Author

bjarnef commented Sep 12, 2023

Hi @emmagarland

Regarding the original use-case I had, I actually used this to convert the display label datetime (depending on backoffice user locale to correct parsing format) as it currently expect the format to be YYYY-MM-DD HH:mm:ss.

Regarding change "label (datetime)" property back to datetime, I think it may be error at server side.

For the label property editor, did you use "Label (DateTime)" and not "Label (String)". Umbraco has some issues, when converting changing datatype to a different property editor using a different value type.

@emmagarland
Copy link
Contributor

@bjarnef yes I did use Label (DateTime). I was wondering if you got the same issue, since I was following your instructions to "Go back to Home Page document type and change End Date property to use standard "Label (datetime)" datatype instead.", so I wanted to check if you got this same issue.

I think it happens on certain dates, such as 25/05/2023, although I don't think it is related to this PR I just wanted to verify behaviour above.

Thanks,

Emma

@bjarnef
Copy link
Contributor Author

bjarnef commented Sep 12, 2023

Hi @emmagarland

When I have a backoffice user (en-US) and select 25/05/2023 (May 25, 2023) and switch the property to "Label (datetime)" this is shown on document:

image

And in the details dialog using parsing format MM/DD/YYYY HH:mm:ss:

const parsingFormat = "MM/DD/YYYY HH:mm:ss";

const d1 = moment(prop.value, parsingFormat).locale(currentUser.locale).format(format);
const d2 = dateHelper.getLocalDate(prop.value, currentUser.locale, format, parsingFormat);
const d3 = dateHelper.getLocalDate(prop.value, currentUser.locale, format);

console.log("d1", d1);
console.log("d2", d2);
console.log("d3", d3);

The variable d3 is not used, but logs "Invalid date", because the raw value 5/25/2023 12:00:00 AM returned from the API doesn't match the default parsing format "YYYY-MM-DD HH:mm:ss".

image

E.g.

moment("5/25/2023 12:00:00 AM", "YYYY-MM-DD HH:mm:ss") // returns false in object _isValid property.
moment("5/25/2023 12:00:00 AM", "YYYY-MM-DD HH:mm:ss").locale("en-US").format("LLL") // returns 'Invalid date'

When I change the backoffice user locale to "en-GB" the display label is different though: 25/05/2023 00:00:00

image

image

Now the variable d3 is valid because of:

moment("25/05/2023 00:00:00", "YYYY-MM-DD HH:mm:ss") // returns true in object _isValid property.
moment("25/05/2023 00:00:00", "YYYY-MM-DD HH:mm:ss").locale("en-US").format("LLL") // returns 'May 20, 2025 12:00 AM'

image

However with d3 and "en-GB" the value returned is of course not correct (the expected date was May 25, 2023).

It is because the raw value "25/05/2023 00:00:00" returned by the property and using "en-GB", the parsing format should not be YYYY-MM-DD HH:mm:ss, but DD/MM/YYYY HH:mm:ss (note I originally tested with "en-US" where the formatted date from the property was MM/DD/YYYY HH:mm:ss).

image

So that's is basically why I needed to specify in the parsing format in the use-case, because I in a custom table view at each row had a button to show details, which fetched data from the existing document and its properties (or some of them), but manipulating the views to the values as labels.

@emmagarland emmagarland merged commit 46b0833 into umbraco:contrib Oct 6, 2023
13 checks passed
@emmagarland
Copy link
Contributor

Hi @bjarnef,

Thanks so much for your explanations and patience with me on this pull request to allow people to specify the format and parsing format in dateHelper functions.

It has now been successfully merged! 🙌

I don't think Bellissima is going to be implementing datetimes in the same way, so I haven't tagged this for v14, but I'm sure you will spot if this isn't the case!

Much appreciate your ongoing efforts,

Best regards

Emma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid date returned from contentResource.getById
3 participants