Skip to content
This repository has been archived by the owner on Mar 25, 2023. It is now read-only.

Add series issue 6 #81

Closed
wants to merge 68 commits into from
Closed

Conversation

kev711
Copy link

@kev711 kev711 commented Mar 21, 2021

Summary of change

If a book is in a series, it should be possible to see:

Its position in the series
com.karankumar.booksapi.repository.BookRepository.getBookPositionInBookSeries()
The name of the series (e.g. 'Harry Potter' for the Harry Potter books)
com.karankumar.booksapi.repository.BookRepository.getAllBookSeriesForBook()
Other books in the same series
com.karankumar.booksapi.repository.BookSeriesMappingRepository.getAllBooksByBookSeries()

Related issue

Closes #6

Pull request checklist

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

  • Read, understood and adhered to our contributing document.

    • Ensure that you were first assigned to a relevant issue before creating this pull request
    • Ensure code changes pass all tests
  • 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-123

  • Set 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 or by creating a new Q&A discussion on GitHub

@knjk04
Copy link
Member

knjk04 commented Mar 22, 2021

Thanks for working on this! I'll review this shortly!

Copy link
Member

@knjk04 knjk04 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I've left a few comments and made a few changes (so I recommend pulling first).

Could you please add a Flyway migration?

@kev711
Copy link
Author

kev711 commented Mar 27, 2021

I am working on resolving the other comments...

@kev711
Copy link
Author

kev711 commented Mar 29, 2021

Thanks for working on this! I've left a few comments and made a few changes (so I recommend pulling first).

Could you please add a Flyway migration?

Hi @knjk04,
I added scripts for Flyway migration. How do you test them ?

Also, made changes for other comments.

@knjk04
Copy link
Member

knjk04 commented Mar 30, 2021

Hi @kev711,

We're using integration tests for that. I noticed you have some DataJpaIntegration tests. I'll need to review this closer, but that should be fine

@kev711
Copy link
Author

kev711 commented Apr 15, 2021

Working on it...

…ies-issue-6

# Conflicts:
#	src/main/java/com/karankumar/booksapi/model/Book.java
#	src/test/java/com/karankumar/booksapi/model/BookSeriesMappingTest.java
#	src/test/java/com/karankumar/booksapi/repository/BookRepositoryTest.java
#	src/test/java/com/karankumar/booksapi/repository/BookSeriesMappingRepositoryTest.java
@kev711
Copy link
Author

kev711 commented Apr 19, 2021

Merge conflicts resolved.

@knjk04
Copy link
Member

knjk04 commented Apr 25, 2021

There are quite a few unrelated changes to the issue you worked on in this PR diff. Would it be possible to create a new PR to make it easier to review?

@kev711
Copy link
Author

kev711 commented Apr 26, 2021

Sure.

@kev711
Copy link
Author

kev711 commented Apr 26, 2021

Closing this one, since its not allowing me to open a new one.

@kev711 kev711 closed this Apr 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add series
3 participants