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

allow other constraint to be filled #356

Open
wants to merge 2 commits into
base: 3.12.x
Choose a base branch
from

Conversation

wangf1122
Copy link
Contributor

@wangf1122 wangf1122 commented Aug 31, 2023

The current validation doesn't allow this other constraint field to be filled if the user didn't select "other restriction" for Access Constraint field or User Constraint field.

image

This pull request removes such barrier. Other validation will still stay (i.e. check other constraint's emptiness if the user select "Other restriction" for Access Constraint field or User Constraint field.)

@wangf1122 wangf1122 marked this pull request as ready for review August 31, 2023 20:41
@ianwallen ianwallen requested review from josegar74 and ianwallen and removed request for josegar74 October 4, 2023 19:32
@ianwallen
Copy link
Contributor

@wangf1122
Could you explain the issue. - Based on my testing I don't see any issue.

The screenshot that you posted will be invalid because you did select other restriction for use constraint or access constraint
Without your change, I can add other constraint using the following sample.

image

@wangf1122
Copy link
Contributor Author

@ianwallen @josegar74

We had discussed this issue during last community meeting session. The issue is this field doesn't allow any input if the user DOES NOT select "other restriction". So the error text is not so clear. I will ask the technical writer to compose another text and update the error itself.

@wangf1122
Copy link
Contributor Author

wangf1122 commented Oct 16, 2023

@josegar74 @ianwallen

There is a logic flaw in the current logic in this area. As we discussed from the last HNAP discussion session, if the user not select "other restriction" but decide to put some text to other constraint field. We should see error message saying this field needs to be empty. But it is pointed to the original none empty message. I added such scenario and message to handle this case. Here is the test result

  1. If Access Constraints or Use Constraints field has no "other restriction", the error will be this
    image

  2. If Access Constraints or Use Constraints field has "other restriction" but the other constraint is empty, then error will be 👍
    image

The code is updated and you can do some general test based on this branch of code. The text update also went thru our technical writer

@@ -45,7 +45,8 @@
<SecurityNoteMismatchedBothLang>Value mismatched for Security User Note in both languages</SecurityNoteMismatchedBothLang>
<SecurityClassificationUserNote>Security User Note is not valid for the classification code selected. Valid values are:</SecurityClassificationUserNote>
<SecurityClassificationUserNoteEmpty>Security User Note should be empty for the classification code selected</SecurityClassificationUserNoteEmpty>
<OtherConstraintsNote>If you indicate 'Other Restrictions' in the 'Access Constraints' or 'Use Constraints' fields, the other constraints for accessing or using the resource should be explained here.</OtherConstraintsNote>
<OtherConstraintsNote>If you have indicated 'Other Restrictions' in either the 'Access Constraints' or 'Use Constraints' fields, the ‘Other constraints' text box must be completed.</OtherConstraintsNote>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not use MS special characters for quote

image

@@ -45,7 +45,8 @@
<SecurityNoteMismatchedBothLang>Value mismatched for Security User Note in both languages</SecurityNoteMismatchedBothLang>
<SecurityClassificationUserNote>Security User Note is not valid for the classification code selected. Valid values are:</SecurityClassificationUserNote>
<SecurityClassificationUserNoteEmpty>Security User Note should be empty for the classification code selected</SecurityClassificationUserNoteEmpty>
<OtherConstraintsNote>If you indicate 'Other Restrictions' in the 'Access Constraints' or 'Use Constraints' fields, the other constraints for accessing or using the resource should be explained here.</OtherConstraintsNote>
<OtherConstraintsNote>If you have indicated 'Other Restrictions' in either the 'Access Constraints' or 'Use Constraints' fields, the ‘Other constraints' text box must be completed.</OtherConstraintsNote>
Copy link
Contributor

@ianwallen ianwallen Oct 17, 2023

Choose a reason for hiding this comment

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

Should " text box must be completed" changed to " text box must be filled".

I don't believe completed the the correct term for filling in a form?

Or it could be rewritten to the following

If you have indicated 'Other Restrictions' in either the 'Access Constraints' or 'Use Constraints' fields, ensure to provide information for 'Other Constraints' field.

@@ -741,13 +741,22 @@
and (../gmd:accessConstraints/gmd:MD_RestrictionCode/@codeListValue = 'RI_609'
or ../gmd:useConstraints/gmd:MD_RestrictionCode/@codeListValue = 'RI_609')) or

(../gmd:accessConstraints/gmd:MD_RestrictionCode/@codeListValue != 'RI_609'
Copy link
Contributor

@ianwallen ianwallen Oct 17, 2023

Choose a reason for hiding this comment

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

I don't understand the extra or condition which seem to indicate that if the other constraints is not specified in 'Access Constraints' or 'Use Constraints' then it is ok to add other constraints. I don't believe this correct...

I believe the user can only add other constraints if they specify other constraints in 'Access Constraints' or 'Use Constraints'

@josegar74
Copy link
Contributor

josegar74 commented Oct 23, 2023

According to the HNAP 2.3.1 specification:

  • accessConstraints (5.4.2.2): For data assessed and approved for release under the Open Government Licence - Canada accessConstraints shall be mandatory and conform to "licence; licence"

  • useConstraints (5.4.2.3): For data assessed and approved for release under the Open Government Licence - Canada useConstraints shall be mandatory and conform to “licence; licence"

  • otherConstraints: otherConstraints shall be provided where accessConstraints (5.4.2.2) or useConstraints (5.4.2.3) is set to "otherRestrictions."


From the previous rules, I understand that otherConstraints is mandatory when accessConstraints or useConstraints are set to "otherRestrictions". In other cases, can be provided optionally.

The original validation, allowed only a value in otherConstraints, if accessConstraints or useConstraints are set to "otherRestrictions", reporting an error in other cases. This seems not correct according to the previous rules.


The pull request keeps the original validation, but provides clear messages. Please @ianwallen and @wangf1122 verify if my previous understanding is correct based on the specification text. If so I think the validation for OtherConstraintsNoteEmpty has to be removed.

In case that OtherConstraintsNoteEmpty should be kept, I noticed this issue with the validation (empty message for the multilingual validation):

validation-empty-message

@ianwallen
Copy link
Contributor

From the previous rules, I understand that otherConstraints is mandatory when accessConstraints or useConstraints are set to "otherRestrictions". In other cases, can be provided optionally.

I agree that there is no specification indicating that otherConstraints cannot be used when accessConstraints or useConstraints are not set to otherRestrictions. So it does seem like it can be optional in this case.

My only concern is how FGP currently handles this situation. I just tested FGP and it fails with this use case so if we implement this change, then FGP also has to implement the change as well.

image

@bo-lu - could you provide your comments on this. Thank you

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.

3 participants