Feat/data exchange request be#1116
Feat/data exchange request be#1116OlgaIvkovic wants to merge 5 commits intoeclipse-tractusx:feat/data-exchange-request-and-approvalfrom
Conversation
7e117d6 to
52ee4c0
Compare
52ee4c0 to
e126630
Compare
|
I've helped fix some git history issues thus my commit. I did not author the actual content of the commit. |
ReneSchroederLJ
left a comment
There was a problem hiding this comment.
Thank you for your contribution. Overall great work. My feedback is mostly down to validations
| @ResponseStatus(HttpStatus.CREATED) | ||
| public OwnDataExchangeRequest createDataExchangeRequest(@RequestBody OwnDataExchangeRequest ownDataExchangeRequest) { | ||
| if (!validator.validate(ownDataExchangeRequest).isEmpty()) { | ||
| throw new ResponseStatusException(HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
| throw new ResponseStatusException(HttpStatus.BAD_REQUEST); | |
| throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid Own Data Exchange Request"); |
| @ApiResponse(responseCode = "500", description = "Internal Server Error.", content = @Content) | ||
| }) | ||
| @ResponseStatus(HttpStatus.CREATED) | ||
| public OwnDataExchangeRequest createDataExchangeRequest(@RequestBody OwnDataExchangeRequest ownDataExchangeRequest) { |
There was a problem hiding this comment.
It would be better to create a DataExchangeRequestDto that is passed here. The DTO should replace notification with notificationId for simplicity.
| } | ||
| if (ownDataExchangeRequest.getNotification() == null || ownDataExchangeRequest.getNotification().getNotificationId() == null) { | ||
| throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Own Data Exchange Request misses notification identification."); | ||
| } |
There was a problem hiding this comment.
This check becomes unnecessary with the DTO change.
|
|
||
| @PrePersist | ||
| protected void onCreate() { | ||
| if (timestamp == null) { |
There was a problem hiding this comment.
This leaves room for timestamp to be manually set. I think it should always overwrite manually set timestamps.
| errors.add(message); | ||
| } | ||
| } | ||
| private void validateDesiredDate(List<String> errors, Date desiredDate, String fieldName, ReportedDemandAndCapacityNotification notification) { |
There was a problem hiding this comment.
I think it would make sense to refactor this method to specifically test both dates together, because currently we're lacking the validation dataExchangeRequest.getDesiredStartDate().before(dataExchangeRequest.getDesiredEndDate())
| } | ||
|
|
||
| public boolean validate(OwnDataExchangeRequest dataExchangeRequest) { | ||
| return validateWithDetails(dataExchangeRequest).isEmpty(); |
There was a problem hiding this comment.
At the moment we're providing the elaborate validation feedback messages, but not using them anywhere. The question is will they be used or would a simpler method of validation akin to the one for notifications suffice.
It is arguable that any time an attempt is made to create or update an entity, this kind of detailed feedback is valuable, however we only use detailed feedback for data imports so far.
I will open a separate issue for a potential refactor, but for the time being I suggest converting to returning a simple boolean.
| } | ||
|
|
||
| public final OwnDataExchangeRequest create(OwnDataExchangeRequest ownDataExchangeRequest) { | ||
| if (!validator.apply(ownDataExchangeRequest)) { |
There was a problem hiding this comment.
| if (!validator.apply(ownDataExchangeRequest)) { | |
| if (ownDataExchangeRequest == null || !validator.apply(ownDataExchangeRequest)) { |
|
|
||
| addIfNull(errors, dataExchangeRequest.getCriticality(), "Missing criticality."); | ||
| addIfNull(errors, dataExchangeRequest.getText(), "Missing text."); | ||
| addIfNull(errors, dataExchangeRequest.getTimestamp(), "Missing timestamp."); |
There was a problem hiding this comment.
This validation for timestamp seems problematic to me. On one hand an existing DataExchangeRequest should always have a valid timestamp. However, this check would require manually setting it before the entity is created even though it will automatically be set at creation. My suggestion would be to remove this check or replace it with something like:
(dataExchangeRequest.getUuid() == null || dataExchangeRequest.getTimestamp() != null)| "n-tier" | ||
| ], | ||
| "text": "Please provide the requested data.", | ||
| "timestamp": "{{DATA_EXCHANGE_TIMESTAMP}}", |
There was a problem hiding this comment.
As previously discussed timestamp should not be manually set sinc eit will be ignored.
|
|
||
| script:post-response { | ||
| console.log("STATUS:", res.getStatus()); | ||
| console.log("BODY:", JSON.stringify(res.getBody(), null, 2)); |
There was a problem hiding this comment.
remove unnecessary scripts
ReneSchroederLJ
left a comment
There was a problem hiding this comment.
Thank you for the changes. I found some further potential improvements. Please also update the version numbers in charts/puris/README.md
| throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Referenced notification does not exist."); | ||
| } | ||
|
|
||
| OwnDataExchangeRequest ownDataExchangeRequest = convertToEntity(requestDto); |
There was a problem hiding this comment.
I'm aware we used to use these kinds of helper methods in the past, but I would opt to use ModelMapper to simplify the process.
This would look something like:
OwnDataExchangeRequest ownDataExchangeRequest = modelMapper.map(requestDto, OwnDataExchangeRequest.class);
ownDataExchangeRequest.setNotification(notification);| ownDataExchangeRequest.setNotification(notification); | ||
|
|
||
| try { | ||
| return convertToDto(ownDataExchangeRequestService.create(ownDataExchangeRequest)); |
There was a problem hiding this comment.
Same as above. This can be handled with ModelMapper:
OwnDataExchangeRequest newEntity = ownDataExchangeRequestService.create(ownDataExchangeRequest);
DataExchangeRequestDto responseDto = modelMapper.map(newEntity, DataExchangeRequestDto.class);
responseDto.setNotificationId(notification.getNotificationId());
return responseDto;| dataExchangeRequest.getNotification() | ||
| ); | ||
| } | ||
| private boolean validateDesiredDates(Date desiredStartDate, Date desiredEndDate, ReportedDemandAndCapacityNotification notification) { |
There was a problem hiding this comment.
Since all 3 parameters are properties of OwnDataExchangeRequest I would opt to pass it directly instead of each individual property.
| if (!desiredStartDate.before(desiredEndDate)) { | ||
| return false; | ||
| } | ||
| if (notification.getStartDateOfEffect() != null) { |
There was a problem hiding this comment.
This check is not necessary since start date is mandatory for notifications
| } | ||
|
|
||
| @Test | ||
| void emptyRequest_testValidateWithDetails_returnsValidationErrors() { |
There was a problem hiding this comment.
fix the function name please due to removal of validateWithDetails
ReneSchroederLJ
left a comment
There was a problem hiding this comment.
I found two very small details to fix. Otherwise good rework.
charts/puris/README.md
Outdated
| # puris | ||
|
|
||
|    | ||
|    |
There was a problem hiding this comment.
The versions should be 7.0.0 and 6.0.0. also please update the badge urls accordingly
| } | ||
| private boolean validateDesiredDates(Date desiredStartDate, Date desiredEndDate, ReportedDemandAndCapacityNotification notification) { | ||
| if (!desiredStartDate.before(desiredEndDate)) { | ||
| private boolean validateDesiredDates(DataExchangeRequest ownDataExchangeRequest, ReportedDemandAndCapacityNotification notification) { |
There was a problem hiding this comment.
Since notification is also a child of dataExchangeRequest it can be omitted from the function parameters as well
Description
Resolves #1090 .
Pre-review checks
Please ensure to do as many of the following checks as possible, before asking for committer review:
changelog.md) with PR reference and brief summary.frontend/package.json,frontend/package-lock.json)backend/pom.xml)scripts/generate_openapi_yaml.pywith running customer backend)