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

Improve HTML validation #3192

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve HTML validation #3192

wants to merge 3 commits into from

Conversation

LTA-Thinking
Copy link
Collaborator

Description

Improves HTML validation with change from #3101 and removing src validation which was preventing most Structure Definitions from being loaded.

Related issues

Addresses #3100

Testing

New/existing tests

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch

@LTA-Thinking LTA-Thinking added Bug Bug bug bug. Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs labels Mar 23, 2023
@LTA-Thinking LTA-Thinking added this to the S111 milestone Mar 23, 2023
@LTA-Thinking LTA-Thinking requested a review from a team as a code owner March 23, 2023 16:31
Comment on lines -293 to -299
if (string.Equals("src", attr.Name, StringComparison.OrdinalIgnoreCase))
{
if (!Src.Any(x => attr.Value.StartsWith(x, StringComparison.OrdinalIgnoreCase)))
{
onInvalidAttr(element, attr);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a few examples to the tests of what this now supports? (i.e. the IG image example) Are there any negative test cases we should add?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there are any negatives to add. @michael-wilson-au added a negative test as part of the first PR, but my change only removes a restriction. I'll add some examples of what is supported.

@LTA-Thinking LTA-Thinking added the KI-Breaking This is a known issue that causes a breaking change label Mar 31, 2023
@LTA-Thinking
Copy link
Collaborator Author

Waiting for customer communication to be planned before merging. As this is a breaking change due to the more strict validation we want to alert customers first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Bug Bug bug bug. KI-Breaking This is a known issue that causes a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants