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

Return remaining V2 fields #513

Merged
merged 1 commit into from
May 4, 2022
Merged

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented May 2, 2022

Why these changes are being introduced:

The data model includes fields that are not yet returned in V2
of the API.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/RDI-89

How this addresses that need:

This returns the additional V2 fields.

Side effects of this change:

  • These fields have been added to the base set of fields, but this
    may require modification once we determine the extended set.
  • The following fields included in this commit are not required for
    RDI: contents, call_numbers, holdings, literary_form,
    numbering, physical_description, `publication_frequency.
  • Cassettes have been regenerated.
  • The id field has been changed to timdex_record_id, as they
    seem to be the same (timdex_record_id is listed as a new field
    in V2 of the data model). I'd like the reviewer to confirm whether
    this is an acceptable approach, or if id should remain as mapped
    from timdex_record_id wih timdex_record_id returned as a separate
    field or not returned at all.

How can a reviewer manually see the effects of these changes?

Some of the new fields will be physical by calling the search endpoint o the V2 API (however, not all of them are represented in our test data yet).

Requires Database Migrations?

NO

Includes new or updated dependencies?

NO

@jazairi
Copy link
Contributor Author

jazairi commented May 2, 2022

This branch has been deployed to TIMDEX staging.

@jazairi jazairi requested review from JPrevost and matt-bernhardt May 2, 2022 20:56
@matt-bernhardt
Copy link
Member

I've gone through the commit, confirmed that the tests still pass, and compared the current state of _base.json.builder to the v2 spec spreadsheet. I can confirm that every field in the spec is now listed in the base partial (and the base partial introduces no field not listed in the spec).

I'm not sure about the timdex_record_id question, though. I understand that breaking changes like renaming this field are permissible, but I'm not sure whether user expectations for an id field are strong enough to justify including this value twice (first as id and then again as timdex_record_id). It is hard for me to anticipate another possible returned ID that could result in another id - like a foo_id field and also a timdex_record_id, both of which would then be duplicated as just id.

I'm going to try and sleep on this, and come at the question tomorrow morning with a fresh mind. I'm posting this comment now to remind future-me of where I left off.

Questions in my mind right now include:

  • Should _base.json.builder` alphabetize earlier fields? I see the new fields have been added in alpha order, but the earlier batch were not.
  • I'm not sure I follow the rationale behind the changes in the cassettes, but this is more likely my end-of-day brain being generally confused.

Why these changes are being introduced:

The data model includes fields that are not yet returned in V2
of the API.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/RDI-89

How this addresses that need:

This returns the additional V2 fields.

Side effects of this change:

* These fields have been added to the base set of fields, but this
may require modification once we determine the extended set.
* The following fields included in this commit are not required for
RDI: `contents`, `call_numbers`, `holdings`, `literary_form`,
`numbering`, `physical_description`, `publication_frequency.
* Cassettes have been regenerated.
* We are continuing to map `timdex_record_id` to `id`, based on
discussion in the PR. We may need to revisit this later on if it proves
confusing either to us or our users.
@jazairi jazairi force-pushed the rdi-89-return-remaining-v2-fields branch from e330af4 to 4db276e Compare May 4, 2022 12:33
@jazairi jazairi merged commit cab0f1b into main May 4, 2022
@jazairi jazairi deleted the rdi-89-return-remaining-v2-fields branch May 4, 2022 12:34
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.

4 participants