Skip to content
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

CCMSPUI-376: Migrate notification summary from SOA-API to EBS-API #119

Merged

Conversation

JamieBriggs-MoJ
Copy link
Contributor

@JamieBriggs-MoJ JamieBriggs-MoJ commented Dec 4, 2024

As part of CBP-947, work was done to move the notification summary from SOA to EBS, which resulted in a new view XXCCMS_NOTIFICATION_COUNT_V being created to support this.

As part of this PR and CCMSPUI-376, a new endpoint has been created which uses the new view.

@JamieBriggs-MoJ JamieBriggs-MoJ self-assigned this Dec 4, 2024
@JamieBriggs-MoJ JamieBriggs-MoJ changed the title CCMSPUI-376: Replace SOA notification summary CCMSPUI-376: Migrate notification summary from SOA-API to EBS-API Dec 5, 2024
Copy link
Contributor

@farrell-m farrell-m left a comment

Choose a reason for hiding this comment

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

Looks good. Might just be personal preference, but I think notifications would sit better under their own controller/service, as those used for User I see as more for user metadata and user management.

Copy link
Contributor

@PhilDigitalJustice PhilDigitalJustice left a comment

Choose a reason for hiding this comment

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

1 minor comment

…being an inner class, and moved notification logic to NotificationsController and it's own services

Signed-off-by: Jamie Briggs <jamie.briggs@digital.justice.gov.uk>
Signed-off-by: Jamie Briggs <jamie.briggs@digital.justice.gov.uk>
Signed-off-by: Jamie Briggs <jamie.briggs@digital.justice.gov.uk>
@JamieBriggs-MoJ JamieBriggs-MoJ marked this pull request as ready for review December 9, 2024 11:16
@JamieBriggs-MoJ
Copy link
Contributor Author

Looks good. Might just be personal preference, but I think notifications would sit better under their own controller/service, as those used for User I see as more for user metadata and user management.

I agree with this retrospectively. I have made some more changes to go in line with this and to decouple the notification summary stuff away from the User controller/service.

Copy link
Contributor

@PhilDigitalJustice PhilDigitalJustice left a comment

Choose a reason for hiding this comment

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

LGTM

@JamieBriggs-MoJ JamieBriggs-MoJ merged commit 18ce086 into main Dec 9, 2024
1 check passed
@JamieBriggs-MoJ JamieBriggs-MoJ deleted the story/ccmspui-376-replace-soa-notification-summary branch December 9, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants