at_client_sdk icon indicating copy to clipboard operation
at_client_sdk copied to clipboard

feat: uptake notify fetch changes

Open sitaram-kalluri opened this issue 2 years ago • 3 comments

- What I did

  • Add 'fetch' method to NotificationService class and fetches the AtNotification from the cloud secondary using the notification id

- How I did it

  • Files Modified:

    • at_notification.dart - When a notification is expired, the notification is removed from the keystore and hence details cannot be fetched to populate the fields in AtNotification class (except the id). So, removed the late modifier and initialized the fields to empty string and epochMillis to '0 (Zero)' to avoid LateInitialization error when accessing the AtNotification.
    • Since AtNotification() has arguments that are available when notification is expired, added a named constructor AtNotification.empty and populate the fields.
    • Added unit tests for the scenario's - when notification is available in keystore - when notification is not available in keystore
    • Removed the skip tag on end2end test and refactored to fetch the notification using notify fetch and added assertions

- How to verify it

- Description for the changelog

sitaram-kalluri avatar Oct 03 '22 13:10 sitaram-kalluri

A few comments /. questions /. changes requested.

Couple of other things:

  • please add to the functional test suite in at_functional_test/test/atclient_notify_test.dart
  • please add functional and end to end tests for fetches on expired notifications
  • please add functional and end to end tests for fetches on non-existent notifications
  • please add unit tests for every exception scenario for fetch
  • Added tests in at_functional_tests
  • Added test in e2e for non-existent notifications
  • Added unit tests for exception scenario
  • At the moment, we do not have a way to set the expiry on the notification. We have a expose-notification-params PR which allows to add expiry to notifications. Can we add fetch notification on expiry in that PR?

sitaram-kalluri avatar Oct 12 '22 09:10 sitaram-kalluri

  • At the moment, we do not have a way to set the expiry on the notification. We have a expose-notification-params PR which allows to add expiry to notifications. Can we add fetch notification on expiry in that PR?

I don't understand ... we can set ttln on a notification. When time-to-live is set, then when the time-to-live duration has passed, the notification has expired, no?

gkc avatar Oct 12 '22 09:10 gkc

  • At the moment, we do not have a way to set the expiry on the notification. We have a expose-notification-params PR which allows to add expiry to notifications. Can we add fetch notification on expiry in that PR?

I don't understand ... we can set ttln on a notification. When time-to-live is set, then when the time-to-live duration has passed, the notification has expired, no?

At the moment, in at_client SDK, we do not have a way to set ttln on the NotificationParams. We have an open PR which exposes notificationExpiry.

sitaram-kalluri avatar Oct 13 '22 02:10 sitaram-kalluri

This LGTM. @VJag also had comments & questions, please don't merge until @VJag has also re-reviewed & approved

Sure Gary

sitaram-kalluri avatar Oct 28 '22 04:10 sitaram-kalluri