-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: implemented the backend for demand and capacity notifications 2.0 #415
feat: implemented the backend for demand and capacity notifications 2.0 #415
Conversation
...va/org/eclipse/tractusx/puris/backend/notification/logic/service/OwnNotificationService.java
Fixed
Show resolved
Hide resolved
...g/eclipse/tractusx/puris/backend/notification/logic/service/ReportedNotificationService.java
Fixed
Show resolved
Hide resolved
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.
Overall great work. Had a few findings.
Sidenote: Please always mark the dependency and license header things in the pr on your own prior to requesting the review :)
backend/src/main/java/org/eclipse/tractusx/puris/backend/common/security/SecurityConfig.java
Outdated
Show resolved
Hide resolved
@ResponseBody | ||
@Operation(summary = "Get all own notifications", description = "Get all own notifications. Optionally the partner can be filtered by its bpnl.") | ||
public List<NotificationDto> getAllNotifications(Optional<String> partnerBpnl) { | ||
if (partnerBpnl.isEmpty()) { |
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 sanitize / pattern check partnerBpnl
.../java/org/eclipse/tractusx/puris/backend/notification/controller/NotificationController.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/eclipse/tractusx/puris/backend/notification/domain/model/Notification.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/eclipse/tractusx/puris/backend/notification/domain/model/Notification.java
Outdated
Show resolved
Hide resolved
...g/eclipse/tractusx/puris/backend/notification/logic/service/ReportedNotificationService.java
Fixed
Show resolved
Hide resolved
...src/main/java/org/eclipse/tractusx/puris/backend/notification/domain/model/Notification.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/eclipse/tractusx/puris/backend/notification/domain/model/Notification.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/eclipse/tractusx/puris/backend/notification/logic/dto/NotificationDto.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/eclipse/tractusx/puris/backend/notification/logic/dto/NotificationDto.java
Outdated
Show resolved
Hide resolved
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 see new comment. Regarding bpnl sanitizing: not sure if you missed the comment, please check. If you propose to not go with sanitizing the controllers parameter, please elaborate why.
...src/main/java/org/eclipse/tractusx/puris/backend/notification/logic/dto/NotificationDto.java
Outdated
Show resolved
Hide resolved
I'm unsure what you mean regarding the BPNL sanitizing. Is the pattern check I added to the parameter not enough? |
Ah sorry, you're right. Got confused due to the outdated conversation. |
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.
LGTM, great contribution! :)
Description
resolves #316
Pre-review checks
Please ensure to do as many of the following checks as possible, before asking for committer review: