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

Change Default Time Zone Handling #363

Merged
merged 6 commits into from
Mar 28, 2022

Conversation

sowu880
Copy link
Contributor

@sowu880 sowu880 commented Mar 21, 2022

  1. Change default time zone handling for FormatAsDateTime() API from "local" to "preserve".
  2. Output of dateTime filters should be valid FHIR format with time zone.

@sowu880 sowu880 force-pushed the personal/sowu/default-timezone-handling branch from 108916b to 2dc1525 Compare March 23, 2022 03:18
@yankunhuang-pku
Copy link
Contributor

yankunhuang-pku commented Mar 23, 2022

Do we need to update the functional test as well? It seems that the pipeline in Mac fails after this change.
Failed Microsoft.Health.Fhir.Liquid.Converter.FunctionalTests.FunctionalTests.GivenHl7v2Message_WhenConverting_ExpectedFhirResourceShouldBeReturned(rootTemplate: "OML_O21", inputFile: "../../data/SampleData/Hl7v2/OML-O21-03.hl7", expectedFile: "TestData/Expected/Hl7v2/OML_O21/OML-O21-03-expected.json")

@yankunhuang-pku
Copy link
Contributor

Why do we need to modify so many sample data and functional tests? It seems that in this pipeline, all functional tests under windows can pass, and only one test in MAC fails

@sowu880
Copy link
Contributor Author

sowu880 commented Mar 25, 2022

Why do we need to modify so many sample data and functional tests? It seems that in this pipeline, all functional tests under windows can pass, and only one test in MAC fails

Since we changed the time zone handling methods( change 'local' to 'preserve'). Although all tests except one passed, the expected result is incorrect base our new datetime handling methods.

@@ -246,6 +246,9 @@ public void GivenCcdaDocument_WhenConverting_ExpectedFhirResourceShouldBeReturne
var expectedContent = File.ReadAllText(expectedFile);
var actualContent = ccdaProcessor.Convert(inputContent, rootTemplate, new TemplateProvider(templateDirectory, DataType.Ccda));

var newPath = expectedFile.Replace("Ccda", "newCcda");
Directory.CreateDirectory(Path.GetDirectoryName(newPath));
File.WriteAllText(newPath, actualContent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

@sowu880 sowu880 force-pushed the personal/sowu/default-timezone-handling branch from 6ef2dfa to 5ce0346 Compare March 25, 2022 08:23
@sowu880
Copy link
Contributor Author

sowu880 commented Mar 28, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@sowu880 sowu880 merged commit 6d9b1cd into dotliquid Mar 28, 2022
@sowu880 sowu880 deleted the personal/sowu/default-timezone-handling branch March 28, 2022 04:23
jakiefermsft added a commit to jasonheld-msft/FHIR-Converter that referenced this pull request Apr 26, 2022
…fer/pgcritiques-merge

* 'main' of github.com:microsoft/FHIR-Converter:
  Global Readiness Manifest file GeoPol.xml
  Update version (microsoft#375)
  Remove blank lines (microsoft#374)
  Update Doc for Filters  (microsoft#365)
  Switch FormatAsDateTime timezone handling to preserve to maintain consistency (microsoft#358)
  Revert "reset to local (microsoft#368)"
  reset to local (microsoft#368)
  Phase -2, Milestone-3 Mapping (microsoft#366)
  Change Default Time Zone Handling (microsoft#363)
  Turn on mac functional test check (microsoft#367)
  Fix warnings (microsoft#364)
  Fix Time Zone Bug (microsoft#353)
  Phase2 - Milestone 1&2 Mapping (microsoft#354)
  Add check for large for loops (microsoft#361)
  Updated the logic of checking the number of default templates
  Update pipeline win host version (microsoft#359)
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.

None yet

5 participants