#17 - Add derived query for ISBN#29
Conversation
|
@ahsanbagwan Thanks for working on this. As per our contributing guidelines, we only accept PRs where you have first been assigned to the issue. Could you please request to be assigned to the issue? Also, I've added the PR checklist back in -- could you please fill it in? |
knjk04
left a comment
knjk04
left a comment
There was a problem hiding this comment.
Thanks for working on this! I've left some comments for you to address
| @Query("SELECT DISTINCT b FROM Book b LEFT JOIN FETCH b.authors WHERE b.isbn13=:isbn") | ||
| Book findBookByISBN(String isbn); |
There was a problem hiding this comment.
Could you please remove the JPQL query?
There was a problem hiding this comment.
Not sure what you mean could you elaborate?
There was a problem hiding this comment.
This line:
@Query("SELECT DISTINCT b FROM Book b LEFT JOIN FETCH b.authors WHERE b.isbn13=:isbn") | bookRepository.save(book); | ||
|
|
||
| Book result = bookRepository.findBookByISBN("978-3-16-148410-0"); | ||
| Assertions.assertEquals(book,result); |
There was a problem hiding this comment.
As per our style guide, we're using AssertJ assertions, not JUnit. Could you please change this?
|
|
||
| private Book createBookwithISBN() { | ||
| Book book = new Book("Game of APIs", new Author[]{author1, author2} , Language.ENGLISH); | ||
| book.setIsbn13("978-3-16-148410-0"); |
There was a problem hiding this comment.
Instead of having the ISBN literal twice, could you please extract this to a field?
|
Thanks for making the changes. Could you also please add to the |
|
Thanks for making the change. Does this work for you? This is the stack trace I get when I run this: https://gist.github.com/knjk04/d5fa917e9da9bd27d302e294fdaf570c |
|
@ahsanbagwan Just following up. Did you manage to get a chance to see the message above? |
|
Oops missed this, will check it out tomorrow.
…On Mon, 28 Dec, 2020, 9:57 pm Karan Kumar, ***@***.***> wrote:
@ahsanbagwan <https://github.com/ahsanbagwan> Just following up. Did you
manage to get a chance to see the message above?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA6M5PYGMDRINLMLLUOTVKDSXCWYFANCNFSM4UQVPB7A>
.
|
…y-isbn-17 # Conflicts: # src/main/resources/graphql/query.graphqls
|
Let us know when you're ready for another review |
|
I'm no longer getting the graphql schema error on running the application... Snip Could you please go ahead and add review comments if required? |
2 participants
Summary of change
Related issue
Closes #17
Pull request checklist
Please keep this checklist in & ensure you have done the following:
Read, understood and adhered to our contributing document.
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.
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
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-123Set this pull request to 'draft' if you are still working on it
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