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

847 record uniqueness #1307

Merged
merged 7 commits into from
Jul 12, 2021
Merged

847 record uniqueness #1307

merged 7 commits into from
Jul 12, 2021

Conversation

duncandewhurst
Copy link
Contributor

@duncandewhurst duncandewhurst commented Jun 15, 2021

Closes #847

records_reference.md:

  • Update the first paragraph to clarify the uniqueness rules for records.
  • Update Record structure to clarify that:
    • the releases list need only contain all releases at the time of the record's publication.
    • the versioned release need only contain a history of changes up to the time of the record's publication.
  • Update Compiled release to clarify that the compiled release need only contain the latest values at the time of the record's publication.

releases_and_records.md

  • Change 'While releases are never updated, records are updated each time there is a change.' to 'While releases are never updated, records can be updated each time there is a change.' to clarify that there is no requirement to update old records.

  • Update .description in record-package-schema.json to describe only a record package, similar to description in release-package-schema.json. Previously, .description also paraphrased the description of a record in definitions.Record.description, possibly because there is no record-schema.json file. However, I think it's best to document the description of a record in only one place in the schema.

  • Update .description in definitions.Record:

    • Align with the first sentence of records_reference.md.
    • Describe only the must/should/may rules for the releases list, compiled release and versioned release.

    Previously, .description also paraphrased the descriptions from .properties.releases, .properties.compiledRelease and .properties.versionedRelease. However, I think it's best to document the descriptions of each property in only one place in the schema.

  • Update .description in definitions.Record.properties.releases to clarify that the releases list need only contain all releases at the time of the record's publication

  • Update .description in definitions.Record.properties.compiledRelease to clarify that the compiled release need only contain the latest values at the time of the record's publication and to replace 'latest version of all the contracting data' with 'latest value of each field'.

  • Update .description in definitions.Record.properties.versionedRelease to clarify that the versioned release need only contain a history of changes up to the time of the record's publication and, since a versioned release can be published independently of a compiled release, replace 'all the data in the compiled release' with 'each field'.

@duncandewhurst duncandewhurst changed the base branch from 1.1-dev to 1.2-dev June 15, 2021 22:45
@duncandewhurst
Copy link
Contributor Author

@jpmckinney, before I request a review from a helpdesk analyst, should we also document the uniqueness rules in the schema by adding the following to the description of Record?

There should be a single record per contracting process per distribution, where a distribution might be a specific API endpoint or a specific bulk download file.

@duncandewhurst duncandewhurst added this to the 1.2.0 milestone Jun 15, 2021
@jpmckinney
Copy link
Member

@duncandewhurst I have not reviewed, but yes, makes sense to also update the schema.

@jpmckinney
Copy link
Member

You'll need to merge 1.2-dev to get tests to pass.

@duncandewhurst duncandewhurst marked this pull request as ready for review June 17, 2021 02:16
Copy link
Contributor

@mrshll1001 mrshll1001 left a comment

Choose a reason for hiding this comment

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

All these changes reflect the clarifications indicated, and are very well written.

@duncandewhurst duncandewhurst requested review from jpmckinney and removed request for odscrachel June 17, 2021 18:56
docs/schema/records_reference.md Outdated Show resolved Hide resolved
docs/schema/records_reference.md Outdated Show resolved Hide resolved
schema/record-package-schema.json Outdated Show resolved Hide resolved
schema/record-package-schema.json Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
# Record Reference

Whereas there can be multiple releases about a contracting process, there should be a single **record** per contracting process, aggregating all the releases available for the contracting process.
A record aggregates the releases related to a contracting process. There should be a single record per contracting process per [distribution](https://www.w3.org/TR/vocab-dcat-2/#Class:Distribution), where a distribution might be a specific API endpoint or a specific bulk download file.
Copy link
Member

Choose a reason for hiding this comment

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

I prefer using "releases about a contracting process" throughout instead of related to, because a release is a data artifact that describes a real-world contracting process (an "about" relationship), whereas "relates to" is much more generic (e.g. a release about a contract renewal could, in a sense, "relate" to the original contract).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Fixed in 5b055f7

@@ -122,7 +122,7 @@
"definitions": {
"Record": {
"title": "Record",
"description": "An OCDS record must provide a list of all the existing OCDS releases related to a single contracting process and should provide a compiled release containing the current state of all fields in the release schema. An OCDS record may also provide a versioned history of all changes to the data in the compiled release.",
"description": "A record aggregates the releases related to a contracting process. A record must include a `releases` list, should include a `compiledRelease` and may include a `versionedRelease`. There should be a single record per contracting process per [distribution](https://www.w3.org/TR/vocab-dcat-2/#Class:Distribution), where a distribution might be a specific API endpoint or a specific bulk download file.",
Copy link
Member

Choose a reason for hiding this comment

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

The "must" statement is enforced by required, which I don't think we typically repeat in field descriptions. I wonder whether the should statement can be moved to the compiledRelease description. The remaining "may" statement is not necessary.

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 guess the original intention was to have all the statements in one place. However, that need is met by records_reference.md so I've made the changes you suggest in 5521210

schema/record-package-schema.json Outdated Show resolved Hide resolved
duncandewhurst and others added 3 commits July 12, 2021 10:00
Co-authored-by: James McKinney <26463+jpmckinney@users.noreply.github.com>
Remove repeated and unnecessary statements from `Record` description. 
Move should statement to `compiledRelease` description.
@jpmckinney
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants