-
Notifications
You must be signed in to change notification settings - Fork 65
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
NOTIF-315 Adds support for default behavior #809
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.
I only had a look at the sessions changes, this looks interesting!
backend/src/main/java/com/redhat/cloud/notifications/db/session/CommonStateSession.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/CommonStateSession.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/CommonStateSessionFactory.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/CommonStateSession.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/CommonStateSession.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/CommonStateSessionFactory.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/Invoker.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/models/EndpointProperties.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.
FYI I will review the code again later, after most of the following comments have been resolved.
backend/src/main/java/com/redhat/cloud/notifications/models/filter/ApiResponseFilter.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/models/BehaviorGroup.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/models/BehaviorGroup.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/CommonStateSession.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Outdated
Show resolved
Hide resolved
backend/src/main/resources/db/migration/V1.34.0__NOTIF-315_default_behavior_group.sql
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/redhat/cloud/notifications/TestConstants.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/redhat/cloud/notifications/processors/email/EmailTest.java
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/EndpointResources.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
Outdated
Show resolved
Hide resolved
Sorry for the delay before I reviewed this BTW 😃 |
@gwenneg I addressed most of the suggested changes |
…a session or stateless session
- Review comments - Example of caching the introspection
Updating tests
backend/src/main/java/com/redhat/cloud/notifications/models/BehaviorGroup.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/models/BehaviorGroup.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/Invoker.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/redhat/cloud/notifications/processors/email/EmailTest.java
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/EndpointResources.java
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/EndpointResources.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Gwenneg Lepage <gwenneg@users.noreply.github.com>
Updated again! |
backend/src/main/java/com/redhat/cloud/notifications/models/EmailSubscriptionProperties.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.
I'm done reviewing the runtime code. I'll have a look at the test tomorrow.
backend/src/main/java/com/redhat/cloud/notifications/db/session/InvokerPrePersist.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/InvokerPrePersist.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/session/InvokerPrePersist.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/redhat/cloud/notifications/TestHelpers.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/redhat/cloud/notifications/events/LifecycleITest.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/com/redhat/cloud/notifications/events/LifecycleITest.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/routers/InternalService.java
Show resolved
Hide resolved
backend/src/main/java/com/redhat/cloud/notifications/db/BehaviorGroupResources.java
Show resolved
Hide resolved
Co-authored-by: Gwenneg Lepage <gwenneg@users.noreply.github.com>
- Fix/add tests
…fecycleITest.java Co-authored-by: Gwenneg Lepage <gwenneg@users.noreply.github.com>
…fecycleITest.java Co-authored-by: Gwenneg Lepage <gwenneg@users.noreply.github.com>
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
The PR check failures seem unrelated. |
This PR adds a behavior group type that is not bound to an specifyc
account_id
. Instead it can be used globally across all accounts.Default behavior group
The only considerated endpoint type for this kind of behavior groups is the
EmailSubscription
, and it needs to be global as well (account_id
should benull
).There are new endpoints in the Internal API for managing this configuration:
/defaultBehaviorGroups
(BehaviorGroup)/defaultBehaviorGroups/{behaviorGroupId}/actions
(List of EmailSubscriptionProperties)/defaultBehaviorGroups/{behaviorGroupId}/eventType/{eventTypeId}
/defaultBehaviorGroups/{behaviorGroupId}/eventType/{eventTypeId}
Some remarks:
default_behavior
. It is true when theaccount_id
isnull
, false otherwise.CommonStateSession
We use stateful sessions when dealing with REST calls and stateless sessions with kakfa events. There are some functions that we need in both places, instead of duplicating the function to run in both scenarios, a wrapper was created for a subset of operations.
The CommonStateSessionFactory provides a method to request a
session
depending our needs - The developer has to specify the state type, it doesn't autodetect anything.Stateless sessions using this tool will attempt to mimic what one could expect with the stateful sessions. Currently it does use introspection to find if there is any method annotated with
@PrePersit
that should be called. This process is cached per class to avoid recomputing this on every call.