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

If a result (baseline|target|actual) value is present, then it must be a number #189

Closed
samuele-mattiuzzo opened this issue Apr 26, 2019 · 7 comments
Labels
question Rules where we have a question

Comments

@samuele-mattiuzzo
Copy link
Contributor

{"if": "result/indicator/baseline/@value", "then": "number(result/indicator/baseline/@value) = number(result/indicator/baseline/@value)"},

{"if": "result/indicator/period/target/@value", "then": "number(result/indicator/period/target/@value) = number(result/indicator/period/target/@value)"},

{"if": "result/indicator/period/actual/@value", "then": "number(result/indicator/period/actual/@value) = number(result/indicator/period/actual/@value)"}

We're checking for numeric values to be numbers in Ruleset but this should instead be enforced at Schema level. We should remove the rules from the Ruleset and the tests for them.

We should also update the Schema type in the relevant places (example -> xsd:string to xsd:decimal)

@andylolz
Copy link
Contributor

@samuele-mattiuzzo It seems like you might be working through this gist, and creating issues. Is that the case?

@samuele-mattiuzzo
Copy link
Contributor Author

we have made a copy of the gist and re-assessed some of the rules you pointed out yes.

I'm also logging issues without necessarily:

  • having finalised all the discussions around them with the team
  • having tagged all the correct versions they need fixing on

@andylolz
Copy link
Contributor

andylolz commented Apr 26, 2019

Great! I didn’t hear anything back regarding the gist, so I wasn’t sure whether you’d looked at it. Great to hear you are going through the points 👍 It was quite a lot of work, so I’m pleased it’s useful.

@samuele-mattiuzzo
Copy link
Contributor Author

Yeah sorry about the silence, but like I said, hectic times :D I forked your gist and used it as a base for our investigations, we also needed to decide whether the effort and time used to fix them was worth it or we should have instead backtracked to pre-January

We learned a bit more about Ruleset, Standard and how to read them through those January changes, so we kept them and decided to fix them. Some are straight forward, some still require some back and forth discussions (eg, this one, as it changes a Schema type effectively enforcing the must be omitted part as a side effect, sort of, so it's sort of sensitive).

Your gist is invaluable btw!

@samuele-mattiuzzo samuele-mattiuzzo moved this from To do to In progress in Post 2019 changes Apr 29, 2019
@samuele-mattiuzzo
Copy link
Contributor Author

samuele-mattiuzzo commented Apr 29, 2019

How this will be changed (@amy-silcock to 👍 / 👎 )

  • we cannot change the Schema because it would prevent 2.02 data from passing 2.03 validation; the number type requiredness is also a should and not a must, one more reason to retain xsd:string as the base type

  • the checks in if then will have to be removed completely for this rule (not the tests as they check for the more generic if-then approach)

  • this is a 2.03 only change

@samuele-mattiuzzo
Copy link
Contributor Author

Relevant PR #204

@samuele-mattiuzzo samuele-mattiuzzo moved this from In progress to Code Complete in Post 2019 changes Apr 29, 2019
@amy-silcock
Copy link
Contributor

Details of the issue and how we propose to fix it can be found here: https://discuss.iatistandard.org/t/bug-fix-the-new-2-03-result-baseline-and-period-value-rules-are-not-backwards-compatible/1731

@Ocre42 Ocre42 moved this from Code Complete to In Review in Post 2019 changes May 1, 2019
@Ocre42 Ocre42 moved this from In Review to Completed in Post 2019 changes May 2, 2019
@Ocre42 Ocre42 moved this from Completed to Code Complete in Post 2019 changes May 2, 2019
@Ocre42 Ocre42 added the on hold label May 2, 2019
@samuele-mattiuzzo samuele-mattiuzzo moved this from Code Complete to In progress in Post 2019 changes May 2, 2019
@samuele-mattiuzzo samuele-mattiuzzo added the question Rules where we have a question label May 2, 2019
@Ocre42 Ocre42 moved this from In progress to Completed in Post 2019 changes May 8, 2019
@Ocre42 Ocre42 removed the on hold label May 8, 2019
@Ocre42 Ocre42 closed this as completed May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Rules where we have a question
Projects
Post 2019 changes
  
Completed
Development

No branches or pull requests

4 participants