Notification dismissal state tracking #5129

samgst-amazon · 2024-11-26T01:37:37Z

The user will be provided with an option to Dismiss the message. Once a user has clicked on Dismiss in any of the UI components, they will not be shown the same notification again.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Description

To keep a track of which notifications are not shown again, notification ids of these messages will be stored to the aws.xml file(persistent storage). This set is retrieved while processing notifications and used to filter out previously dismissed notifications from being further processed and shown.

Checklist

My code follows the code style of this project
I have added tests to cover my changes
A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2024-11-26T01:56:03Z

Qodana Community for JVM

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

Or via our issue tracker: https://jb.gg/qodana-issue
Or share your feedback: https://jb.gg/qodana-discussions

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt

...mmunity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPollingService.kt

...community/src/software/aws/toolkits/jetbrains/core/notifications/ProcessNotificationsBase.kt

manodnyab · 2024-11-26T02:14:51Z

...community/src/software/aws/toolkits/jetbrains/core/notifications/ProcessNotificationsBase.kt

+                ?: Pair(emptyList(), emptyList())
+
+            val dismissalState = NotificationDismissalState.getInstance()
+            (if (isStartup) startupNotifications else emptyList())


Should this part handle the fact that startup notifications should be shown only once per session?

nit: can we move the handling into separate list handling for readability

yes and yes, I was passing the boolean in from the polling service but I think the flag can live here instead.

rli · 2024-11-26T17:22:02Z

plugins/core/jetbrains-community/src/software/aws/toolkits/jetbrains/utils/NotificationUtils.kt

        createNotificationExpiringAction(
            object : AnAction("Dismiss") {
                override fun actionPerformed(e: AnActionEvent) {
                    ProcessNotificationsBase.showBannerNotification.remove(id)


state should not be stored in companion objects

rli · 2024-11-26T17:23:38Z

...community/src/software/aws/toolkits/jetbrains/core/notifications/ProcessNotificationsBase.kt

+    private val project: Project,
+) {
    private val notifListener = mutableListOf<NotifListener>()
+    private var isStartup: AtomicBoolean = AtomicBoolean(true)


wouldn't you want startup notifications to propagate consistently across all projects on startup?

...ity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationServiceInitializer.kt

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt

rli · 2024-11-26T17:24:45Z

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt

+        fun getInstance(): NotificationEtagState =
+            ApplicationManager.getApplication().getService(NotificationEtagState::class.java)


Suggested change

fun getInstance(): NotificationEtagState =

ApplicationManager.getApplication().getService(NotificationEtagState::class.java)

fun getInstance(): NotificationEtagState =

ApplicationManager.getApplication().getService<NotificationEtagState>()

I'm getting a syntax error when I do this. I think this is for newer versions of Intellij SDK. I don't see any other usages of the bottom code in our repository, but I do see the ::class.java version.

can you try using getInstance(): NotificationETagState = service()

that works!

rli · 2024-11-26T17:25:36Z

...community/src/software/aws/toolkits/jetbrains/core/notifications/ProcessNotificationsBase.kt

+        notifications?.let { notificationsList ->
+            val (startupNotifications, emergencyNotifications) = notificationsList.notifications
+                ?.partition { notification ->
+                    notification.schedule.type.equals("StartUp", ignoreCase = true)


should be modeled in the deserializer

rli · 2024-11-26T17:26:26Z

plugins/core/src/main/resources/META-INF/plugin.xml

        <applicationService serviceImplementation="software.aws.toolkits.jetbrains.core.notifications.NotificationEtagState"/>
+        <applicationService serviceImplementation="software.aws.toolkits.jetbrains.core.notifications.NotificationDismissalState"/>


these can be light services

rli · 2024-11-26T17:27:37Z

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt

+
+@State(name = "notificationDismissals", storages = [Storage("aws.xml")])
+class NotificationDismissalState : PersistentStateComponent<NotificationDismissalConfiguration> {
+    private var state = NotificationDismissalConfiguration()


rli · 2024-11-26T17:27:41Z

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt

+
+@State(name = "notificationEtag", storages = [Storage("aws.xml")])
+class NotificationEtagState : PersistentStateComponent<NotificationEtagConfiguration> {
+    private var state = NotificationEtagConfiguration()


rli · 2024-11-26T17:27:56Z

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt

+import com.intellij.openapi.components.State
+import com.intellij.openapi.components.Storage
+
+@State(name = "notificationDismissals", storages = [Storage("aws.xml")])


consider using BaseState

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt

rli · 2024-11-27T00:16:10Z

...ty/src/software/aws/toolkits/jetbrains/core/notifications/NotificationCustomDeserializers.kt

+// Create a custom deserializer if needed
+class NotificationTypeDeserializer : JsonDeserializer<NotificationScheduleType>() {
+    override fun deserialize(p: JsonParser, ctxt: DeserializationContext): NotificationScheduleType =
+        NotificationScheduleType.fromString(p.valueAsString)
+}
+


not convinced we actually need a custom deserializer but ok

* initial commit * run on startup * detekt * move vals * remote resource implementation * comments * detekt * Validate file before saving * cache path * observer implementation * deserialize notifs from file * detekt * remove unused interface * internal class * Fix observer * etag singleton state component * add telemetry * atomicBoolean * initialize once per IDE startup * code scan * Omit (Unit) * specify etag storage location * detekt * fix detekt issues * basic tests * no star imports * coroutine scope delay instead of thread.sleep * feedback fixes * test fix * Application Exists for tests * endpoint object * detekt * detekt fixes * boolean flag * boolean flag * update tests * move startup flag handling to processBase * fix delay * fix delay * Notification dismissal state tracking (#5129) * split notifications into separated lists. * add persistent notification dismissal state logic * boolean changes * group persistant states * comments * Service initialized automatically * isStartup global * Deserialized notification schedule type * tests * persistent state syntax * convert to light services * Remove state from companion object * detekt * endpoint as registryKey * detekt * fix startup issues * Expiry issues

* Add framework for processing notifications (#5112) * Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (#5093) * Display toast notifications with actions * Condition matcher for displaying notifications * Modified deserialization cases and added tests * not required file change * feedback 1 * modified the base class * modified test instance lifecycle * Show toasts for notifications and notification banner on critical notifications(#5097) * Display toast notifications with actions * Condition matcher for displaying notifications * Show notification banner * feedback 1 * Modified deserialization cases and added tests * not required file change * feedback 1 * feedback 1 * modified the base class * merge conflicts resolved * rearranged call site * show notifications when panel is opened * fixed tests * detekt * feedback * convert panels into wrappers * fixed test * Adding update/restart action to notifications (#5136) * Poll for new notifications (#5119) * initial commit * run on startup * detekt * move vals * remote resource implementation * comments * detekt * Validate file before saving * cache path * observer implementation * deserialize notifs from file * detekt * remove unused interface * internal class * Fix observer * etag singleton state component * add telemetry * atomicBoolean * initialize once per IDE startup * code scan * Omit (Unit) * specify etag storage location * detekt * fix detekt issues * basic tests * no star imports * coroutine scope delay instead of thread.sleep * feedback fixes * test fix * Application Exists for tests * endpoint object * detekt * detekt fixes * boolean flag * boolean flag * update tests * move startup flag handling to processBase * fix delay * fix delay * Notification dismissal state tracking (#5129) * split notifications into separated lists. * add persistent notification dismissal state logic * boolean changes * group persistant states * comments * Service initialized automatically * isStartup global * Deserialized notification schedule type * tests * persistent state syntax * convert to light services * Remove state from companion object * detekt * endpoint as registryKey * detekt * fix startup issues * Expiry issues * Add logging info to IDE notification polling and processing (#5138) * add logs for polling and processing notifs * redundant * finish log * fix isFirstPoll not setting to false on first pass --------- Co-authored-by: aws-toolkit-automation <[email protected]> Co-authored-by: manodnyab <[email protected]> Co-authored-by: Bryce Ito <[email protected]>

* Add framework for processing notifications (#5112) * Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (#5093) * Display toast notifications with actions * Condition matcher for displaying notifications * Modified deserialization cases and added tests * not required file change * feedback 1 * modified the base class * modified test instance lifecycle * Show toasts for notifications and notification banner on critical notifications(#5097) * Display toast notifications with actions * Condition matcher for displaying notifications * Show notification banner * feedback 1 * Modified deserialization cases and added tests * not required file change * feedback 1 * feedback 1 * modified the base class * merge conflicts resolved * rearranged call site * show notifications when panel is opened * fixed tests * detekt * feedback * convert panels into wrappers * fixed test * Adding update/restart action to notifications (#5136) * Poll for new notifications (#5119) * initial commit * run on startup * detekt * move vals * remote resource implementation * comments * detekt * Validate file before saving * cache path * observer implementation * deserialize notifs from file * detekt * remove unused interface * internal class * Fix observer * etag singleton state component * add telemetry * atomicBoolean * initialize once per IDE startup * code scan * Omit (Unit) * specify etag storage location * detekt * fix detekt issues * basic tests * no star imports * coroutine scope delay instead of thread.sleep * feedback fixes * test fix * Application Exists for tests * endpoint object * detekt * detekt fixes * boolean flag * boolean flag * update tests * move startup flag handling to processBase * fix delay * fix delay * Notification dismissal state tracking (#5129) * split notifications into separated lists. * add persistent notification dismissal state logic * boolean changes * group persistant states * comments * Service initialized automatically * isStartup global * Deserialized notification schedule type * tests * persistent state syntax * convert to light services * Remove state from companion object * detekt * endpoint as registryKey * detekt * fix startup issues * Expiry issues * Add logging info to IDE notification polling and processing (#5138) * add logs for polling and processing notifs * redundant * finish log * show message instead of yesNoDialogue * initialize ProcessNotificationsBase listener before Polling service * fix isFirstPoll not setting to false on first pass --------- Co-authored-by: aws-toolkit-automation <[email protected]> Co-authored-by: manodnyab <[email protected]> Co-authored-by: Bryce Ito <[email protected]>

* Add framework for processing notifications (aws#5112) * Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (aws#5093) * Display toast notifications with actions * Condition matcher for displaying notifications * Modified deserialization cases and added tests * not required file change * feedback 1 * modified the base class * modified test instance lifecycle * Show toasts for notifications and notification banner on critical notifications(aws#5097) * Display toast notifications with actions * Condition matcher for displaying notifications * Show notification banner * feedback 1 * Modified deserialization cases and added tests * not required file change * feedback 1 * feedback 1 * modified the base class * merge conflicts resolved * rearranged call site * show notifications when panel is opened * fixed tests * detekt * feedback * convert panels into wrappers * fixed test * Adding update/restart action to notifications (aws#5136) * Poll for new notifications (aws#5119) * initial commit * run on startup * detekt * move vals * remote resource implementation * comments * detekt * Validate file before saving * cache path * observer implementation * deserialize notifs from file * detekt * remove unused interface * internal class * Fix observer * etag singleton state component * add telemetry * atomicBoolean * initialize once per IDE startup * code scan * Omit (Unit) * specify etag storage location * detekt * fix detekt issues * basic tests * no star imports * coroutine scope delay instead of thread.sleep * feedback fixes * test fix * Application Exists for tests * endpoint object * detekt * detekt fixes * boolean flag * boolean flag * update tests * move startup flag handling to processBase * fix delay * fix delay * Notification dismissal state tracking (aws#5129) * split notifications into separated lists. * add persistent notification dismissal state logic * boolean changes * group persistant states * comments * Service initialized automatically * isStartup global * Deserialized notification schedule type * tests * persistent state syntax * convert to light services * Remove state from companion object * detekt * endpoint as registryKey * detekt * fix startup issues * Expiry issues * Add logging info to IDE notification polling and processing (aws#5138) * add logs for polling and processing notifs * redundant * finish log * fix isFirstPoll not setting to false on first pass --------- Co-authored-by: aws-toolkit-automation <[email protected]> Co-authored-by: manodnyab <[email protected]> Co-authored-by: Bryce Ito <[email protected]>

* Add framework for processing notifications (aws#5112) * Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (aws#5093) * Display toast notifications with actions * Condition matcher for displaying notifications * Modified deserialization cases and added tests * not required file change * feedback 1 * modified the base class * modified test instance lifecycle * Show toasts for notifications and notification banner on critical notifications(aws#5097) * Display toast notifications with actions * Condition matcher for displaying notifications * Show notification banner * feedback 1 * Modified deserialization cases and added tests * not required file change * feedback 1 * feedback 1 * modified the base class * merge conflicts resolved * rearranged call site * show notifications when panel is opened * fixed tests * detekt * feedback * convert panels into wrappers * fixed test * Adding update/restart action to notifications (aws#5136) * Poll for new notifications (aws#5119) * initial commit * run on startup * detekt * move vals * remote resource implementation * comments * detekt * Validate file before saving * cache path * observer implementation * deserialize notifs from file * detekt * remove unused interface * internal class * Fix observer * etag singleton state component * add telemetry * atomicBoolean * initialize once per IDE startup * code scan * Omit (Unit) * specify etag storage location * detekt * fix detekt issues * basic tests * no star imports * coroutine scope delay instead of thread.sleep * feedback fixes * test fix * Application Exists for tests * endpoint object * detekt * detekt fixes * boolean flag * boolean flag * update tests * move startup flag handling to processBase * fix delay * fix delay * Notification dismissal state tracking (aws#5129) * split notifications into separated lists. * add persistent notification dismissal state logic * boolean changes * group persistant states * comments * Service initialized automatically * isStartup global * Deserialized notification schedule type * tests * persistent state syntax * convert to light services * Remove state from companion object * detekt * endpoint as registryKey * detekt * fix startup issues * Expiry issues * Add logging info to IDE notification polling and processing (aws#5138) * add logs for polling and processing notifs * redundant * finish log * show message instead of yesNoDialogue * initialize ProcessNotificationsBase listener before Polling service * fix isFirstPoll not setting to false on first pass --------- Co-authored-by: aws-toolkit-automation <[email protected]> Co-authored-by: manodnyab <[email protected]> Co-authored-by: Bryce Ito <[email protected]>

samgst-amazon added 5 commits November 25, 2024 14:50

split notifications into separated lists.

8e9325c

split notifs into two lists

ff7debe

add persistent notification dismissal state logic

f2072e4

boolean changes

5009de3

group persistant states

9b60583

github-advanced-security bot found potential problems Nov 26, 2024

View reviewed changes

manodnyab reviewed Nov 26, 2024

View reviewed changes

samgst-amazon changed the base branch from feature/ideNotifs to samgst/ideNotifs-polling November 26, 2024 02:19

samgst-amazon changed the base branch from samgst/ideNotifs-polling to samgst/notificationPolling November 26, 2024 02:20

samgst-amazon marked this pull request as ready for review November 26, 2024 02:25

samgst-amazon requested a review from a team as a code owner November 26, 2024 02:25

comments

34cbdfa

manodnyab approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'samgst/notificationPolling' into samgst/notificationStates

71e90d3

rli reviewed Nov 26, 2024

View reviewed changes

samgst-amazon added 5 commits November 26, 2024 10:37

Service initialized automatically

45bc05d

isStartup global

37b18f9

Deserialized notification schedule type

54799ef

tests

81fa23a

persistent state syntax

e15c0a4

github-advanced-security bot found potential problems Nov 26, 2024

View reviewed changes

...munity/src/software/aws/toolkits/jetbrains/core/notifications/NotificationPersistantState.kt Fixed Show fixed Hide fixed

samgst-amazon added 3 commits November 26, 2024 15:42

convert to light services

911b61e

Remove state from companion object

594ecff

detekt

3e1725f

rli approved these changes Nov 27, 2024

View reviewed changes

samgst-amazon merged commit 26caf31 into samgst/notificationPolling Nov 27, 2024
1 of 7 checks passed

samgst-amazon deleted the samgst/notificationStates branch November 27, 2024 00:28

		fun getInstance(): NotificationEtagState =
		ApplicationManager.getApplication().getService(NotificationEtagState::class.java)

		<applicationService serviceImplementation="software.aws.toolkits.jetbrains.core.notifications.NotificationEtagState"/>
		<applicationService serviceImplementation="software.aws.toolkits.jetbrains.core.notifications.NotificationDismissalState"/>

Notification dismissal state tracking #5129

Notification dismissal state tracking #5129

Uh oh!

Conversation

samgst-amazon commented Nov 26, 2024

Types of changes

Description

Checklist

License

Uh oh!

github-actions bot commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Qodana Community for JVM

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Nov 26, 2024 •

edited

Loading