Navigation Menu

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

Adds ability to apply facets in GraphQL #350

Merged
merged 1 commit into from Oct 19, 2020
Merged

Conversation

JPrevost
Copy link
Member

Our initial GraphQL implementation was intended as experiemental so it did not incldue the ability to apply facets.

As we start to consider whether we will move to GraphQL fully or stay with REST, it seemed worthwhile to consider how working with facets would feel in GraphQL both in terms of consuming the data and writing the code.

https://mitlibraries.atlassian.net/browse/DISCO-60

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

Using the GraphQL Playground in this PR, you should note the Docs have been updated to reflect the new ability to optionally apply facets. Some facets, such as Subject allow an array of Strings and others just allow Strings.

An example starting point might be:

{
  search(searchterm: "spaceflight",
    	format: ["Print volume"]
  	) {
    hits
    aggregations {
    	source {
        key
        docCount
      }
      languages {
        key
        docCount
      }
      contributors {
        key
        docCount
      }
      contentFormat {
        key
        docCount
      }
      subjects {
        key
        docCount
      }
      literaryForm {
        key
        docCount
      }
    }
    records {
      source
      title
      contentType
      format
      contributors {
        kind
        value
      }
      subjects
      literaryForm
      languages
    }
  }
}

What are the relevant tickets?

Requires Database Migrations?

NO

Includes new or updated dependencies?

NO

@JPrevost
Copy link
Member Author

A separate commit will follow with tests but I wanted to get some feedback as-is.

@JPrevost
Copy link
Member Author

There is inconsistent naming for format versus contentFormat which I think I'll open a separate ticket on.

@gravesm gravesm temporarily deployed to timdex-stage-pr-350 September 23, 2020 19:20 Inactive
@JPrevost JPrevost temporarily deployed to timdex-stage-pr-350 September 23, 2020 21:08 Inactive
@JPrevost JPrevost temporarily deployed to timdex-stage-pr-350 September 25, 2020 17:16 Inactive
@JPrevost JPrevost temporarily deployed to timdex-stage-pr-350 September 25, 2020 17:37 Inactive
@gravesm gravesm temporarily deployed to timdex-stage-pr-350 October 1, 2020 20:46 Inactive
@gravesm gravesm temporarily deployed to timdex-stage-pr-350 October 9, 2020 21:20 Inactive
@matt-bernhardt
Copy link
Member

I'm trying to apply facets and better understand how this is supposed to work, and I think I'm misunderstanding something. If I understand this feature correctly, a query like this would return only nonfiction records? When I try it in the playground, though, I'm seeing results that are both nonfiction and fiction. I see similar behavior when trying to facet on languages.

# Write your query or mutation here
{
  search(
    searchterm: "spaceflight",
    literaryForm: "nonfiction"
  ) {
    aggregations {
      literaryForm {
        key
        docCount
      }
    }
    records {
      identifier
      title
      literaryForm
    }
  }
}

Screen Shot 2020-10-09 at 5 32 24 PM

@JPrevost
Copy link
Member Author

@matt-bernhardt I'm seeing the same behavior. I swear this worked at some point ;). I'll try to figure out what broke between then and now and update. Thanks.

@JPrevost
Copy link
Member Author

@matt-bernhardt the last commit broke the functionality. I think I didn't update the test cassettes so didn't notice it. Ooops. I'll fix it... possibly by removing the last commit that broke it and open some tickets to review the CodeClimate concerns if I don't find a quick fix for why it broke.

Copy link
Contributor

@jazairi jazairi left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to sign off on this yesterday afternoon. Following the latest commit, faceting works for me on a few test queries (languages, format, literary form, subjects), and the tests pass. :shipit:

Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

This looks good to me as well. I agree with resolving the conflict between format and contentFormat when we get a chance.

:shipit:

Our initial GraphQL implementation was intended as experiemental so it
did not incldue the ability to apply facets.

As we start to consider whether we will move to GraphQL fully or stay
with REST, it seemed worthwhile to consider how working with facets
would feel in GraphQL both in terms of consuming the data and writing
the code.

https://mitlibraries.atlassian.net/browse/DISCO-60
@JPrevost JPrevost merged commit d70908b into master Oct 19, 2020
@JPrevost JPrevost deleted the DISCO-60_graphql_facets branch October 19, 2020 18:18
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.

None yet

4 participants