Skip to content

Conversation

@jazairi
Copy link
Contributor

@jazairi jazairi commented Jan 26, 2023

Why these changes are being introduced:

Many of the GraphQL fields are not documented.

Relevant ticket(s):

How this addresses that need:

This adds descriptions to all V2 fields and search arguments, using the TIMDEX Data Model v2.0 document as a guide.

Side effects of this change:

  • Descriptions have been modified from the data model where applicable.
  • Listing record sources is a little unwieldy now that we have six of them, but I still think it's useful to include here. I'd be interested in ideas of how better to display this information.
  • One field, 'numbering', does not have a description in the data model. I've set this aside until we have a better understanding of what it does.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

@jazairi jazairi force-pushed the timx-116-add-descriptions branch from 5ce7bda to e685b85 Compare January 26, 2023 17:48
@jazairi jazairi temporarily deployed to timdex-pr-642 January 26, 2023 17:48 Inactive
@jazairi jazairi requested a review from JPrevost January 26, 2023 17:49
@JPrevost JPrevost self-assigned this Jan 26, 2023
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

I think we need to drill down and document the additional nested fields and aggregations, but I'd be okay with doing that as a second pass and working to land this initial round first.

None of my comments are blocking.

field :physical_description, String, null: true, description: 'Physical description of item'
field :publication_frequency, [String], null: true,
description: 'Publication frequency of item (used for serials)'
field :numbering, String, null: true
Copy link
Member

Choose a reason for hiding this comment

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

@hakbailey do you have suggestion as to what wording we might use to describe numbering? It wasn't defined in the data model documents used to add descriptions to the other fields.

@jazairi jazairi requested a review from JPrevost January 26, 2023 22:35
@jazairi jazairi temporarily deployed to timdex-pr-642 January 26, 2023 22:35 Inactive
@jazairi
Copy link
Contributor Author

jazairi commented Jan 26, 2023

@JPrevost I just pushed another commit to address your feedback and document subfields and aggregations. Please let me know if I missed anything, and if any of the subfield descriptions can be improved (some are better than others).

Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

Couple more small suggested changes. I'm super excited to have these descriptions in the spec. Thanks!

@jazairi jazairi requested a review from JPrevost January 27, 2023 15:01
@jazairi jazairi temporarily deployed to timdex-pr-642 January 27, 2023 15:02 Inactive
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

Once this merges go ahead and run the Deploy Jekyll with GitHub Pages dependencies preinstalled action and choose the main branch so we can see the new ref docs build with these new descriptions (and so you can see how that process works first hand).

Why these changes are being introduced:

Many of the GraphQL fields are not documented.

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/TIMX-116

How this addresses that need:

This adds descriptions to all V2 fields and search arguments, using
the TIMDEX Data Model v2.0 document as a guide.

Side effects of this change:

* Descriptions have been modified from the data model where
applicable, or newly written where unavailable (specifically,
subfields and aggregations).
* One field, 'numbering', does not have a description in the data
model. I've set this aside until we have a better understanding
of what it does.
@jazairi jazairi force-pushed the timx-116-add-descriptions branch from 5712eed to 432bf01 Compare January 27, 2023 15:14
@jazairi jazairi temporarily deployed to timdex-pr-642 January 27, 2023 15:14 Inactive
@jazairi jazairi merged commit a193271 into main Jan 27, 2023
@jazairi jazairi deleted the timx-116-add-descriptions branch January 27, 2023 15:16
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.

3 participants