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

Improving SetProperty for DatePicker control to be consistent across every timezone #308

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NavneetThekkumpat
Copy link
Contributor

@NavneetThekkumpat NavneetThekkumpat commented Sep 18, 2023

  • Currently with the SetProperty() function when used to set Date value no timezone is specified and hence Date.parse() converts the date to unix timestamp for the local time.
  • This could cause discrepancy when the value of the DatePicker control is fetched, where in the value is in UTC . So in first step in a different timezone like IST, if the user sets the date to value lets say Date(2029/11/05). Here in this step the getProperty() will get the value in UTC that is 2029/11/04.
    Example tests that passes.
 SetProperty(DatePicker1.SelectedDate, Date(2029, 11, 05));
 Assert(DatePicker1.SelectedDate = Date(2029, 11, **04**), "Validate date was changed");
  • To avoid this discrepancy, we need to make sure that the SetProperty() sets date time value in UTC. Hence the datepicker control date value is consistent with the UTC format, the below test runs successful.
 SetProperty(DatePicker1.SelectedDate, Date(2029, 11, 05));
 Assert(DatePicker1.SelectedDate = Date(2029, 11, 05), "Validate date was changed");
  • The changes here assures if the user setProperty() to set date value for datepicker and tries to Assert the value, then it is consistent, since its always saved in the UTC format.

References https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse

Checklist

  • The code change is covered by unit tests. I have added tests that prove my fix is effective or that my feature works
  • I have performed end-to-end test locally.
  • New and existing unit tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I used clear names for everything
  • I have performed a self-review of my own code

@NavneetThekkumpat NavneetThekkumpat requested a review from a team as a code owner September 18, 2023 14:52
@NavneetThekkumpat NavneetThekkumpat changed the title Updating setproperty for date value to set date to utc time Improving SetProperty for Datepicker to be consistent across Timezone Sep 18, 2023
@NavneetThekkumpat NavneetThekkumpat changed the title Improving SetProperty for Datepicker to be consistent across Timezone Improving SetProperty for DatePicker control to be consistent across every timezone Sep 18, 2023
@NavneetThekkumpat NavneetThekkumpat self-assigned this Sep 18, 2023
// Setting the time value to T00:00.000Z to maintain uniformity across timezones.
// This value explicitly specifies the UTC timezone.
// This helps in fetching the date value as it is in any timezone locally without considering one-off in day value.
var dt = $"Date.parse(\"{recordValue.Date.ToString("yyyy-MM-dd")}{UTCTimeValue}\")";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled in the JSSDK?

Copy link
Contributor Author

@NavneetThekkumpat NavneetThekkumpat Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could potentially change it there too. I thought since we have an option to pass the right value here much earlier in the lifecycle(we have separate handlers for each value type setProperty , i.e date, record, table etc). Are you concerned about how secure this string is ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not worried about the security since there isn't any new user-provided strings. I'm just curious about the trade-offs between forcing a user to update the Test Engine versus having to republish their app to get the fix. Just wanting to make sure you've considered the implications of each of these options & that this is the correct one... (not sure if there was an Analyze on this, if so, I apologize if this was discussed already...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, good point. Considering the easiest option for users. Here since the changes are in the TestEngine code and not in jssdk, user would have to just get the latest TE.

// This value explicitly specifies the UTC timezone.
// This helps in fetching the date value as it is in any timezone locally without considering one-off in day value.
var dt = $"Date.parse(\"{recordValue.Date.ToString("yyyy-MM-dd")}{UTCTimeValue}\")";
var expression = $"PowerAppsTestEngine.setPropertyValue({itemPathString},{{{propertyNameString}:{dt}}})";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other places where this might be called other than the DatePicker which might expect the time to be included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is specific and only to SetProperty with date value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't think I was clear. Might it be possible that someone is calling SetProperty on a control that isn't a DatePicker? For example, is there a control which might be expecting the Time to be included & not replaced with 0000Z?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah interesting. With the known scenarios so far we do have such properties that takes in date value(for setProperty) other than date picker control.
If such a scenario exist or is possible I would say it would be ideal to make these changes in the jssdk and make this condition specific to datepicker control.

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Microsoft.PowerApps.TestEngine 91% 87% 982
Summary 91% (2543 / 2794) 87% (598 / 684) 982

Minimum allowed line rate is 85%

@NavneetThekkumpat NavneetThekkumpat marked this pull request as draft September 19, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants