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: add optional param encryptValue to notify method #1391

Merged
merged 14 commits into from
Sep 13, 2024

Conversation

murali-shris
Copy link
Member

@murali-shris murali-shris commented Sep 9, 2024

Fixed #1390
- What I did

  • encrypt notification value only if metadata.isEncrypted is set to true.
  • ability for caller to not encrypt notification value by setting optional param encryptValue = false in notify method
  • upgraded atsign package dependencies to latest version
    - How I did it
  • added optional param encryptValue in notify method. set atkey.metadata.isEncrypted to the optional param value.
  • changes in notification request transformer to encrypt notification value only if isEncrypted is set to true.
  • added unit and functional test
    - How to verify it
  • unit and functional tests should pass
  • ran tests in at_onboarding_cli
    Test atclient notify change at_libraries#659
  • ran test in sshnoports
    Test at commons notify noports#1348

@murali-shris murali-shris marked this pull request as ready for review September 9, 2024 10:00
@@ -93,6 +93,7 @@ abstract class NotificationService {
true, // this was the behaviour before introducing this parameter
bool checkForFinalDeliveryStatus =
true, // this was the behaviour before introducing this parameter
bool encryptValue = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the comment // this was the behaviour before introducing this parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -319,6 +319,7 @@ class NotificationServiceImpl
true, // this was the behaviour before introducing this parameter
bool checkForFinalDeliveryStatus =
true, // this was the behaviour before introducing this parameter
bool encryptValue = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the comment // this was the behaviour before introducing this parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@gkc gkc 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 @murali-shris - can you please add an e2e test also?

@murali-shris
Copy link
Member Author

Looks good @murali-shris - can you please add an e2e test also?

sure Gary

@murali-shris
Copy link
Member Author

Looks good @murali-shris - can you please add an e2e test also?

done

print(notificationListJson);
expect(notificationListJson[0]['from'], currentAtSign);
expect(notificationListJson[0]['to'], sharedWithAtSign);
expect(notificationListJson[0]['value'], value);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also add an expect for isEncrypted please?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am getting isEncrypted=true for this test.
[{id: 79a2d96e-ff34-442c-aac0-97a556fcbcfc, from: @ce2e1, to: @ce2e2, key: @ce2e2:phone2a9c23ff-cb4c-4d41-a50d-276d87d24820.e2e_test@ce2e1, value: +1 100 200 30, operation: update, epochMillis: 1726120153595, messageType: MessageType.key, isEncrypted: true, metadata: {encKeyName: null, encAlgo: null, ivNonce: null, skeEncKeyName: null, skeEncAlgo: null, availableAt: null, expiresAt: 2024-09-13 05:49:13.595Z***]
Is this because we don't have the changes on the server yet?

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

LGTM

@murali-shris murali-shris merged commit 93acfbe into trunk Sep 13, 2024
10 checks passed
@murali-shris murali-shris deleted the notification_service_encryptvalue branch September 13, 2024 06:45
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.

add optional param encryptValue to NotificationService notify method
2 participants