-
Notifications
You must be signed in to change notification settings - Fork 106
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
Feature #46 CAPTURE_DURATION_OF_STAY #202
Feature #46 CAPTURE_DURATION_OF_STAY #202
Conversation
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
============================================
+ Coverage 69.01% 69.31% +0.29%
- Complexity 198 202 +4
============================================
Files 32 32
Lines 823 844 +21
Branches 37 38 +1
============================================
+ Hits 568 585 +17
- Misses 244 248 +4
Partials 11 11
Continue to review full report at Codecov.
|
@Giluerre Can you please refactor it to use the new endpoints as described in #46 (comment)? Current implementation is not so RESTFul. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the changes as per the newly updated issue description.
@jmprathab Yes, I can. I apologize for the inactivity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole code needs the correct formatting.
There are many comments, please read them and feel free to raise a new PR with proper code, thanks!
/houses/{houseId}/rentals: | ||
post: | ||
tags: | ||
- house-controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rentals
tags: | ||
- house-controller | ||
description: Add the duration of stay to house history. | ||
operationId: captureStay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this method be more meaningful?
description: If parameters are invalid | ||
get: | ||
tags: | ||
- house-controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rentals
get: | ||
tags: | ||
- house-controller | ||
operationId: getHouseHistory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getHouseHistory
?
Maybe it would be better to name it like getRentalHistoryForHouse
?
required: false | ||
responses: | ||
'200': | ||
description: "if history is present" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quotes can be omitted. Start the sentence with uppercase
@OneToOne | ||
@JoinColumn(name = "Id") | ||
private CommunityHouse communityHouse; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank lines
@Repository | ||
public interface HouseHistoryRepository extends PagingAndSortingRepository<HouseHistory,Long> { | ||
|
||
List<HouseHistory> findByHouseId(String houseId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at FIND vs GET naming convention:
(1, 'f71ea3a1-fe94-4f73-9d5d-6e830df42c5e', '2020-10-10 10:30', '2020-10-10 11:00', 0, 1); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed I guess
@@ -115,4 +121,16 @@ public boolean deleteMemberFromHouse(String houseId, String memberId) { | |||
houseMemberRepository.findAllByCommunityHouse_Community_Admins_UserId(userId, pageable) | |||
); | |||
} | |||
|
|||
@Override public Optional<List<HouseHistory>> getHouseHistory(String memberId, String houseId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put override in new line, please.
@@ -16,6 +19,7 @@ | |||
import java.util.UUID; | |||
import java.util.stream.Collectors; | |||
import java.util.stream.Stream; | |||
import javax.rmi.CORBA.Util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this import is there?
Closing - too many code issues. |
🚀 Description
Feature adds
HouseHistory entity
Two new Endpoints:
POST: /houses/member/captureStay
add stay of duration
GET /houses/{houseId}/houseHistory
find all records where houseId is same, Query param memberId is optional.
New methods in Housecontroller are migrated to openAPI
📄 Motivation and Context
close #46
🧪 How Has This Been Tested?
Feature was tested by sending requests via Postman.
📷 Screenshots (if appropriate)
📦 Types of changes
✅ Checklist