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

[Accelerator 4] Adding real time event notification #193

Closed
wants to merge 9 commits into from

Conversation

Ashi1993
Copy link
Contributor

@Ashi1993 Ashi1993 commented Nov 11, 2024

Adding real time event notification

This PR adds Realtime Event Notification Implementation

Issue link: https://github.com/wso2-enterprise/solutions-bfsi/issues/20

Doc Issue: Optional, link issue from documentation repository

Applicable Labels: Spec, product, version, type (specify requested labels)


Development Checklist

  1. Build complete solution with pull request in place.
  2. Ran checkstyle plugin with pull request in place.
  3. Ran Findbugs plugin with pull request in place.
  4. Ran FindSecurityBugs plugin and verified report.
  5. Formatted code according to WSO2 code style.
  6. Have you verified the PR doesn't commit any keys, passwords, tokens, usernames, or other secrets?
  7. Migration scripts written (if applicable).
  8. Have you followed secure coding standards in WSO2 Secure Engineering Guidelines?

Testing Checklist

  1. Written unit tests.
  2. Verified tests in multiple database environments (if applicable).
  3. Tested with BI enabled (if applicable).

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ashi1993 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Ashi1993 Ashi1993 marked this pull request as ready for review December 9, 2024 10:13
</Import-Package>
<DynamicImport-Package>*</DynamicImport-Package>
Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline, let's create an issue to remove this dynamic imports line in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue #218


/**
* Method to update the notification status by ID, allowed values are.
* OPEN,ACK and ERR
Copy link
Contributor

Choose a reason for hiding this comment

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

if only these values are allowed shouldn't we accept an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIxed in 00babc1

// the notification status will be updated as ERROR.
realtimeNotificationService.updateNotificationStatusById(notificationId,
EventNotificationConstants.ERROR);
} catch (IOException | InterruptedException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if these both exception are logging the same error we can merge them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 00babc1

@Ashi1993
Copy link
Contributor Author

Closing this PR and added this again because #224 due to an issue with CLA.

@Ashi1993 Ashi1993 closed this Dec 12, 2024
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.

5 participants