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.

Sign up for GitHub

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

Jump to bottom

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
Draft

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5153b4
Updating setproperty for date value to set date to utc time
NavneetThekkumpat Sep 18, 2023
fa9163f
refactoring comments
NavneetThekkumpat Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/Microsoft.PowerApps.TestEngine.Tests/PowerApps/PowerAppFunctionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ public async Task SetPropertyDateAsyncTest()
var result = await powerAppFunctions.SetPropertyAsync(itemPath, DateValue.NewDateOnly(dt.Date));

Assert.True(result);
MockTestInfraFunctions.Verify(x => x.RunJavascriptAsync<bool>($"PowerAppsTestEngine.setPropertyValue({itemPathString},{{\"SelectedDate\":Date.parse(\"{((DateValue)DateValue.NewDateOnly(dt.Date)).GetConvertedValue(null)}\")}})"), Times.Once());
var dateTimeValue = convertDateToDateTimeUTC(dt);
MockTestInfraFunctions.Verify(x => x.RunJavascriptAsync<bool>($"PowerAppsTestEngine.setPropertyValue({itemPathString},{{\"SelectedDate\":Date.parse(\"{dateTimeValue}\")}})"), Times.Once());
}

[Fact]
Expand Down Expand Up @@ -823,7 +824,8 @@ public async Task SetPropertyDateAsyncNoPublishedAppFunction()
var powerAppFunctions = new PowerAppFunctions(MockTestInfraFunctions.Object, MockSingleTestInstanceState.Object, MockTestState.Object);
await Assert.ThrowsAsync<Exception>(async () => { await powerAppFunctions.SetPropertyDateAsync(itemPath, DateValue.NewDateOnly(dt.Date)); });

MockTestInfraFunctions.Verify(x => x.RunJavascriptAsync<bool>($"PowerAppsTestEngine.setPropertyValue({itemPathString},{{\"SelectedDate\":Date.parse(\"{((DateValue)DateValue.NewDateOnly(dt.Date)).GetConvertedValue(null)}\")}})"), Times.Once());
var dateTimeValue = convertDateToDateTimeUTC(dt);
MockTestInfraFunctions.Verify(x => x.RunJavascriptAsync<bool>($"PowerAppsTestEngine.setPropertyValue({itemPathString},{{\"SelectedDate\":Date.parse(\"{dateTimeValue}\")}})"), Times.Once());
LoggingTestHelper.VerifyLogging(MockLogger, ExceptionHandlingHelper.PublishedAppWithoutJSSDKMessage, LogLevel.Error, Times.Once());
}

Expand Down Expand Up @@ -864,5 +866,9 @@ public async Task GetPropertyAsyncNoPublishedAppFunction()
}

// End Published App JSSDK not found tests
private string convertDateToDateTimeUTC(DateTime dt)
{
return $"{DateValue.NewDateOnly(dt.Date).GetConvertedValue(null).ToString("yyyy-MM-dd")}{PowerAppFunctions.UTCTimeValue}";
}
}
}
10 changes: 8 additions & 2 deletions src/Microsoft.PowerApps.TestEngine/PowerApps/PowerAppFunctions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public class PowerAppFunctions : IPowerAppFunctions
public static string PublishedAppIframeName = "fullscreen-app-host";
public static string CheckPowerAppsTestEngineObject = "typeof PowerAppsTestEngine";
public static string CheckPowerAppsTestEngineReadyFunction = "typeof PowerAppsTestEngine.testEngineReady";
public static string UTCTimeValue = "T00:00:00.000Z";

private string GetItemCountErrorMessage = "Something went wrong when Test Engine tried to get item count.";
private string GetPropertyValueErrorMessage = "Something went wrong when Test Engine tried to get property value.";
Expand Down Expand Up @@ -275,8 +276,13 @@ public async Task<bool> SetPropertyDateAsync(ItemPath itemPath, DateValue value)
var propertyNameString = JsonConvert.SerializeObject(itemPath.PropertyName);
var recordValue = value.GetConvertedValue(null);

// Date.parse() parses the date to unix timestamp
var expression = $"PowerAppsTestEngine.setPropertyValue({itemPathString},{{{propertyNameString}:Date.parse(\"{recordValue}\")}})";
// Date.parse() parses the date to unix timestamp.
// SetProperty value expected from user is Date(yyyy, MM, dd)
// 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 •
edited
Loading

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.

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.


return await _testInfraFunctions.RunJavascriptAsync<bool>(expression);
}
Expand Down
Loading