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

MVP: Schemas for Roofing Tiles/Slate + Prodcom #1

Merged
merged 8 commits into from
May 31, 2023

Conversation

MebinAbraham
Copy link
Contributor

@MebinAbraham MebinAbraham commented May 5, 2023

What is the context of this PR?

Initial PR to add unit level JSON schema definitions for Roofing Tiles, Slate and Prodcom surveys.

Ensure the initial draft makes sense.

Other SDS surveys will be added at a future date, this will formalise the main structure and expected patterns.

Links

Checklist

  • Changes to the spec have been added to example schemas in examples/
  • JSON Schema definitions have been updated

docs/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

Looks good, validation working and examples schemas passing, just some minor comments

scripts/validateSchemas.js Outdated Show resolved Hide resolved
schemas/roofing_tiles_and_slate.json Outdated Show resolved Hide resolved
docs/prodcom.md Outdated Show resolved Hide resolved
docs/roofing_tiles_and_slate.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
schemas/prodcom.json Outdated Show resolved Hide resolved
schemas/template_v1.json Outdated Show resolved Hide resolved
@MebinAbraham
Copy link
Contributor Author

MebinAbraham commented May 15, 2023

@jon-acker @Jon-Kent @martyncolmer @tidmanmike @ONSdigital/eq-runner

Change request to existing implementation:

At the moment, the top level and repeating items have a key_field property which is used to handle the dynamic nature of the models and also support social surveys in future. However, if we standardise the model and say all models must have an identifier property at top level and repeating level, then the key_field property can be dropped. This would make things simpler for eQ Runner and should make look up easier for SDS.

The work required by SDX shouldn't change much either way. Let me know your thoughts. If there is agreement, I will push the changes.

Note we may have to keep key_field at the top-level for data integrity checks if eQ is required to validate the response with the launch claims. This is unknown at the moment.

Before:

{
  "schema_version": "v1",
  "key_field": "ru_ref",
  "ru_ref": "50000035606",
  "items": {
    "local_units": {
      "key_field": "lu_ref",
      "values": [
        {
          "lu_ref": "3340224",
          "lu_name": "STUBBS BUILDING PRODUCTS LTD",
          "lu_address": [
            "WELLINGTON ROAD",
            "LOCHMABEN",
            "SWINDON",
            "BEDS",
            "GLOS",
            "DE41 2WA"
          ]
        },
        {
          "lu_ref": "20047673",
          "lu_name": "HOPSCOTCH INDUSTRIES UK LTD",
          "lu_address": [
            "SOUTH CERNEY WORKS",
            "SHAWELL LANE",
            "BEENHAM",
            "STAFFS",
            "BEDFORDSHIRE",
            "GL4 5YU"
          ]
        }
      ]
    }
  }
}

After:

{
  "schema_version": "v1",
  "identifier": "50000035606",
  "items": {
    "local_units": [
      {
        "identifier": "3340224",
        "lu_name": "STUBBS BUILDING PRODUCTS LTD",
        "lu_address": [
          "WELLINGTON ROAD",
          "LOCHMABEN",
          "SWINDON",
          "BEDS",
          "GLOS",
          "DE41 2WA"
        ]
      },
      {
        "identifier": "20047673",
        "lu_name": "HOPSCOTCH INDUSTRIES UK LTD",
        "lu_address": [
          "SOUTH CERNEY WORKS",
          "SHAWELL LANE",
          "BEENHAM",
          "STAFFS",
          "BEDFORDSHIRE",
          "GL4 5YU"
        ]
      }
    ]
  }
}

Copy link

@kylelawsonAND kylelawsonAND left a comment

Choose a reason for hiding this comment

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

Understood and approved in current impl - will re-review if key_field removal proposal is implemented.

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

All makes sense and happy with current proposal, with or without key field, though would also favour just having the identifier

@tidmanmike
Copy link

  "identifier": "50000035606",

Happy with this approach as it gives it a global unique identifier across both social and business surveys.

@MebinAbraham
Copy link
Contributor Author

FYI, I will be making the changes suggested here #1 (comment). Since @jon-acker, @tidmanmike and Runner team are happy with this, I will go ahead with the changes. The change in structure shouldn't really impact Author.

@MebinAbraham
Copy link
Contributor Author

Model has been updated to use #1 (comment)

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Change to identifier made everywhere key field was previously and has been done as outlined in the comment, looks good

Copy link

@kylelawsonAND kylelawsonAND left a comment

Choose a reason for hiding this comment

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

Change to using identifier looks good and is updated throughout 👍

Copy link

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

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

Re-read the changes seems good to me

Copy link

@martyncolmer martyncolmer left a comment

Choose a reason for hiding this comment

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

Happy with this

Copy link

@martyncolmer martyncolmer left a comment

Choose a reason for hiding this comment

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

Happy with this

Copy link

@jon-acker jon-acker left a comment

Choose a reason for hiding this comment

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

lgtm

@MebinAbraham MebinAbraham merged commit 5c71e89 into main May 31, 2023
@MebinAbraham MebinAbraham deleted the schemas-for-slate-tiles-prodcom branch May 31, 2023 13:26
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.

10 participants