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

Error with validation checks for Release IDs #127

Closed
mrshll1001 opened this issue Dec 17, 2019 · 11 comments · Fixed by #161
Closed

Error with validation checks for Release IDs #127

mrshll1001 opened this issue Dec 17, 2019 · 11 comments · Fixed by #161
Labels
triage The appropriate label and repository is not yet determined

Comments

@mrshll1001
Copy link

In OCDS the guidance for release IDs is given as follows:

A release identifier must be unique within the scope of the contracting process of which it is a part. In other words, across all OCDS releases with the same ocid value, each release identifier refers to exactly one release; no two releases use the same release identifier.

A release identifier must also be consistent within this scope. For example, if the id of a release is “12345” in one release package, then the id of the same release in another release package must also be “12345”.

This means two releases may have the same release id if they are under different contracting processes (OCIDs) and still be valid. In this gist there are 11 releases under 11 different OCIDs, which all happen to share a release ID. The expected behaviour is that the release ids are not flagged as invalid however CoVE is flagging this as a problem.

@Bjwebb
Copy link
Contributor

Bjwebb commented Dec 20, 2019

Note: CoVE's behaviour reflects what the 1.0 and 1.1 standard used to say open-contracting/standard#455

Which is not to say we shouldn't change it now, of course.

@robredpath
Copy link
Contributor

@duncandewhurst
Copy link
Contributor

Noting that this came up again with another publisher, so it would be good to fix.

@Bjwebb
Copy link
Contributor

Bjwebb commented Sep 30, 2020

@mrshll1001 @duncandewhurst How does this look for the error if the combination is non-unqiue? I'm looking especially for feedback on the Examples column, is seperating with a comma sufficiently obvious as to what's going on?

Screenshot from 2020-09-30 17-35-52

@duncandewhurst
Copy link
Contributor

Shouldn't this error be reported under 'Conformance (rules)' rather than Structural Errors?

We could then have a more verbose error, e.g.

1 of your ocids has duplicate release identifier(s). (view all error(s)) ^

ocid release id
ocds-123456-001 001
ocds-123456-001 001
ocds-123456-001 002
ocds-123456-001 002

A release identifier must be unique within the scope of the contracting process of which it is a part. In other words, across all OCDS releases with the same ocid value, each release identifier refers to exactly one release; no two releases use the same release identifier.

@Bjwebb
Copy link
Contributor

Bjwebb commented Oct 1, 2020

"Structural Errors" is what we call errors that have come out of schema validation, and the unique IDs check happens as part of schema validation.

Strictly speaking the schema doesn't describe the ID uniqueness, but it does say that each whole release must be unique. However, checking this for a lot of laregish releases is computationally expensive. Therefore, we override this particular aspect of schema validation, so that if there are IDs, we check those first, because its quicker (but if there aren't IDs we do the slower check). Identical whole releases must necessarily have the same IDs (but not vice versa), so our check is stricter than normal schema validation.

Because we've implemented this by overriding the json schema validator for uniqueness, the errors come out with the other validation errors. However, it would still be possible to keep this implementation the same, but move it in the frontend. I'm not sure if this is a good idea or not. (If we just do that, the CLI would stay the same, as well as the errors that go into kingfisher).

@duncandewhurst
Copy link
Contributor

Ah, I see. I thought it would be an additional check since it involves looking at two fields.

I think this error benefits from a more verbose explanation in the CoVE front-end because of the extra complexity, so I'd favour moving it to conformance errors.

For the CLI and Kingfisher, since the audience for those is mostly helpdesk analysts I think it's fine to keep it as a structural error with the shorter description - as we can still report it to publishers as a conformance issue.

Adding @jpmckinney in case he has a view

@jpmckinney
Copy link
Member

I have no strong feeling. It occurs rarely, so it's not terrible to leave it as-is, but I agree that it'd be clearer to have a longer explanation.

@robredpath robredpath transferred this issue from OpenDataServices/cove Oct 14, 2020
@robredpath robredpath transferred this issue from another repository Oct 14, 2020
@jpmckinney jpmckinney added the check results Relating to how specific checks are reported label Oct 14, 2020
@jpmckinney jpmckinney changed the title OCDS: Error with validation checks for Release IDs Error with validation checks for Release IDs Oct 14, 2020
@jpmckinney
Copy link
Member

@Bjwebb To understand the state of the issue: Has the check been updated in lib-cove-ocds, and now we're just discussing how it's reported, or does that first step still need to be done? (I can then rename and/or transfer the issue.)

@jpmckinney jpmckinney added triage The appropriate label and repository is not yet determined and removed check results Relating to how specific checks are reported labels Oct 14, 2020
@Bjwebb
Copy link
Contributor

Bjwebb commented Oct 15, 2020

@Bjwebb
Copy link
Contributor

Bjwebb commented May 19, 2021

Bjwebb added a commit that referenced this issue May 25, 2021
.mo files are now committed in lib-cove-web.
Fixes open-contracting/deploy#269

Fix decimal bug.
Fixes #158

Check combination of ocid and id for uniqueness in releases.
Fixes #127
Bjwebb added a commit that referenced this issue May 25, 2021
.mo files are now committed in lib-cove-web.
Fixes open-contracting/deploy#269

Fix decimal bug.
Fixes #158

Check combination of ocid and id for uniqueness in releases.
Fixes #127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage The appropriate label and repository is not yet determined
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants