Fix find publisher for book 87

[maines-pet]

Summary of change

Change the publisher field in the graphql schema for book into an array of publishers so that findAllBooks query will be able to return the associated publisher/s of a book.

Related issue

Closes #87

Pull request checklist

Please keep this checklist in & ensure you have done the following:

• [x] Read, understood and adhered to our contributing document.

  • [x] Ensure that you were first assigned to a relevant issue before creating this pull request
  • [x] Ensure code changes pass all tests

• [x] Read, understood and adhered to our style guide. A lot of our code reviews are spent on ensuring compliance with our style guide, so it would save a lot of time if this was adhered to from the outset.

• [x] Filled in the summary, context (if applicable) and related issue section. Replace the square brackets and its placeholder content with your contents. For an example, see any merged in pull request

• [x] Created a branch that has a descriptive name (what your branch is for in a few words and includes the issue number at the end, e.g. test-reading-goal-123

• [x] Set this pull request to 'draft' if you are still working on it

• [x] Resolved any merge conflicts

For any of the optional checkboxes (e.g. the screenshots one), still check it if it does not apply.

If in doubt, get in touch with us via our Slack workspace or by creating a new Q&A discussion on GitHub

[maines-pet]

I'm getting this permission error in Sonarqube when running the build in Github Actions.

Error: Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.9.0.2155:sonar (default-cli) on project booksapi: You're not authorized to run analysis. Please contact the project administrator. -> [Help 1]

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[knjk04]

Hi @maines-pet,

Thanks for working on this! Unfortunately, this does not fit our needs. While a book can have multiple publishers, we may consider that a different edition or book entirely since its ISBN would also be different.

Thanks for letting me know about the failing build. It was a permission error that I've fixed now.

I'll close this PR as it's not what we're looking for. I appreciate that can be quite annoying after you've worked on this. In the future, perhaps you could discuss your implementation approach with us first to prevent this from happening. Hopefully, you'll still stick around to work on other issues!

[maines-pet]

Thanks for the feedback @knjk04. My bad for not clarifying the requirements on the issue.

Thanks for working on this! Unfortunately, this does not fit our needs. While a book can have multiple publishers, we may consider that a different edition or book entirely since its ISBN would also be different.

I can see that the model for the Book.java has a field for the collection of publishers. With your comment above, is the correct approach to change the collection of publishers field into a single publisher instead? Or something else?

[knjk04]

I can see that the model for the Book.java has a field for the collection of publishers. With your comment above, is the correct approach to change the collection of publishers field into a single publisher instead?

Yes, you're right. That shouldn't a one-to-one relationship (we don't need a book on the publisher side). Could you raise a new PR for this? Apologies for the confusion, I can see now why you thought the graphql schema should have been changed.

[maines-pet]

Hi @knjk04, I have identified the issue and has the fix ready. Before I push the commit, can you clarify the below please?

1. The book.graphql has the following field definition:

    publisher: [Publisher!]

    Should I rename it as publishers and set the type as [Publisher!]! (similar to authors field)

2. I have used the query findByPublisher(name:"Bloomsbury) but it looks like there's no book associated to it in the current seed data. I have tested using publisher Penguin and it returns the correct books. Is that the expected result?

[knjk04]

Hi @maines-pet,

1. That's right

2. My bad. We've changed how we're inserting into the publisher_book bridge table and there's not currently an entry for 'Bloombury'. That's good if 'Penguin' works

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[maines-pet]

@knjk04, thanks for confirming. PR is now ready for review.