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

feat: NotificationService #63

Open
wants to merge 30 commits into
base: trunk
Choose a base branch
from

Conversation

JeremyTubongbanua
Copy link
Member

@JeremyTubongbanua JeremyTubongbanua commented Oct 12, 2022

Change Log

  • AtClient has a getNotificationService method to get the NotificationService instance that is instantiated in the constructor of AtClientImpl
  • AtClientValidation.java has a new method validateNotificationRequest(NotificationParams, String rootDomain, int rootPort)
  • under common/, we have NotificationEnums.java which has enums used by Notifications.
  • New NotifyListResponseTransformer that transforms a response from running the verb notify:list into usable Java objects
  • Refactored NotificationStatusVerbBuilder to NotifyStatusVerbBuilder
  • New api/notification/ directory which contains the following:
    • AtNotification.java - object represents an AtNotification
    • NotificationParams.java - object to enter inside of the notify method
    • NotificationResult.java - object returned upon using NotificationService methods that holds information after executing said method.
    • NotificationService.java - interface defining functions to be implemented by Impl
    • NotificationServiceImpl.java - implementation of Notification
    • Tests for NotifyListResponseTransformer (ensure a given notify:list input creates a desired output)

Not Done

  • AtClientValidation tests for checking the NotificationParams object are commented out in AtClientValidationTest.java
  • validateNotificationRequest is not used by NotificationServiceImpl and untested

Usage

See NotifyListExample.java, NotifyRemoveExample.java, and NotifySendExample.java for usage examples.

closes #6

@JeremyTubongbanua JeremyTubongbanua changed the title feat: Java notification feat: NotificationService Dec 19, 2022
@JeremyTubongbanua
Copy link
Member Author

JeremyTubongbanua commented Dec 20, 2022

@gkc

I have a blocker with the last 2 outstanding tasks to merge this PR. They are related to validating the NotificationParams object. In my opinion, I think it is okay to ignore for now because the only work that is left is validating the NotificationParams object. Notifications still work in this PR. I've left comments and the ground work is laid out in case we really want to add Notification validation in the future. If you think this PR is okay, it is good to merge.

More Info on the Issue:

Unable to implement the use of static method AtClientValidation.validateNotificationRequest(NotificationParams, String, int) in line 49 of NotificationServiceImpl.java

The summary of the problem is that the method requires the RootURL of the atDirectory server but the AtClient instance does not have access to this variable because the core factory method (that actually instantiates the AtClientImpl instance) does not store it because it takes the remote secondary address as an argument (as opposed to the root url).

Sources:

  1. Dart AtClientValidation using AtClientPreference to get rootDomain and rootPort https://github.com/atsign-foundation/at_client_sdk/blob/f8998895e11a4a45c8869a2c7433b5a140a85c4b/packages/at_client/lib/src/util/at_client_validation.dart#L133
  2. Dart NotificationServiceImpl actually using the method https://github.com/atsign-foundation/at_client_sdk/blob/f8998895e11a4a45c8869a2c7433b5a140a85c4b/packages/at_client/lib/src/service/notification_service_impl.dart#L339
  3. Java AtClient factory method that actually creates the instance
    static AtClient withRemoteSecondary(AtSign atSign, Secondary.Address remoteSecondaryAddress, boolean verbose) throws AtException {
    (note that this method does not ask for a rootUrl, there are other factory methods that ask for a rootUrl as an argument but eventually call this method).

@JeremyTubongbanua JeremyTubongbanua marked this pull request as ready for review December 20, 2022 01:00
@gkc
Copy link
Contributor

gkc commented May 15, 2023

Branch has merge conflicts that need to be resolved

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.

Implement notify in Java SDK
2 participants