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

bug fix - nested restrictions #236

Merged
merged 2 commits into from
Nov 18, 2024
Merged

bug fix - nested restrictions #236

merged 2 commits into from
Nov 18, 2024

Conversation

leoraba
Copy link
Contributor

@leoraba leoraba commented Oct 31, 2024

PR Changes include:

@joneubank
Copy link
Contributor

We should have a test to capture this issue. Since we don't can you add one?

@@ -165,6 +167,34 @@ describe('Parse Values - parseFieldValue', () => {
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringNoRestriction).success).true;
expect(parseFieldValue(' !@#$%^&* ()_+ ', fieldStringNoRestriction).data).equals('!@#$%^&* ()_+');
});
it('Successfuly parses strings, with conditional restrictions', () => {
const value = 'any random string value!!!';
const result = parseFieldValue(value, fieldStringConditionalExists);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

testing parsing any string using a Schema Field Definition having if/then/else restrictions

});
it('Successfuly parses strings, with conditional restrictions without then or else', () => {
const value = 'any random string value!!!';
const result = parseFieldValue(value, fieldStringConditionalExistsWithouthThenElse);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing parsing any string using a Schema Field Definition having anif restriction without then/else. This is not a functional Schema definition however to cover every scenario possible we have to test it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the expected behaviour that an if without then/else just never applies restrictions?

Should we consider adding validation to lectern dictionaries to require an if to have a then or else (at least one of the two)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not then/else is found in the schema definition the expected behaviour is do not apply the restriction. Based on test resolveFieldRestrictions.spec.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to add a validation to require at least one between then/else. Although it could be added in a separate PR

@leoraba
Copy link
Contributor Author

leoraba commented Nov 14, 2024

PR updated. Adding Unit tests to verify this functionality.
Code coverage is now 100% for matchCodeListFormatting.ts file

Copy link
Contributor

Choose a reason for hiding this comment

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

typo in file name: Withouth

});
it('Successfuly parses strings, with conditional restrictions without then or else', () => {
const value = 'any random string value!!!';
const result = parseFieldValue(value, fieldStringConditionalExistsWithouthThenElse);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the expected behaviour that an if without then/else just never applies restrictions?

Should we consider adding validation to lectern dictionaries to require an if to have a then or else (at least one of the two)?

@leoraba leoraba merged commit 39fb5a8 into develop Nov 18, 2024
2 checks passed
@leoraba leoraba deleted the bug_nested_restriction branch November 18, 2024 21:59
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