Skip to content

Conversation

@kelaompachai
Copy link
Contributor

@kelaompachai kelaompachai commented May 5, 2025

Update client notifications to pull from github Release API

  • Added library java-semver-0.10.2.jar for version parsing and comparison.
  • Refactored to use streams and added tests
  • No longer calls a self-hosted service, so active extensions are no
    longer provided as part of the request.

@kelaompachai kelaompachai marked this pull request as draft May 5, 2025 04:32
@kelaompachai kelaompachai changed the title Issue #24 Closes #24 May 5, 2025
@kpalang kpalang changed the title Closes #24 Notifications May 6, 2025
Copy link
Contributor

@jonbartels jonbartels left a comment

Choose a reason for hiding this comment

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

Add .vscode and .project to .gitignore

Thank you for the PR!

@rogin
Copy link
Contributor

rogin commented May 8, 2025

I've cleaned up the code and added tests. I've drafted a PR to @kelaompachai 's main.

@kelaompachai
Copy link
Contributor Author

I rebased to get rid of the out-of-scope commits that had to do with .project, .vscode, .gitignore.

Copy link
Contributor

@jonbartels jonbartels left a comment

Choose a reason for hiding this comment

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

Several Java 8 compatibility issues.

@rogin
Copy link
Contributor

rogin commented May 17, 2025

I'm out of gas trying to merge correctly. I have java 8 compatible code sitting on this commit. I'd appreciate help, otherwise it's dead on the vine.

@kelaompachai
Copy link
Contributor Author

I got rogin's java 8 compatible code working on my machine and pushed it up into this pr.

@kelaompachai
Copy link
Contributor Author

I did have to manually edit the server/.classpath file to get it to work.

@kelaompachai kelaompachai marked this pull request as ready for review May 18, 2025 04:34
@jonbartels
Copy link
Contributor

I did some testing on Saturday.


Built from keloampachi main branch. Did not mess with versions or any defaults
No notifications shown; no errors

Correction needed - there are no notifications to show, then do not show the notification popup.

image



Manually forcing the version to 3.11 shows

Links are clickable

This test case passed this is the main workflow. GJ!

Improvement requested:
I think the PR should show more data like we see from https://github.com/nextgenhealthcare/connect/releases/tag/4.4.2

Mirth Connect 4.4.2 is a patch release that includes bug fixes and an improvement to exporting messages.
Then the whats new, upgrade, and release links.

Is that data included in the payload returned from git or would it require a second fetch?

image


@kelaompachai
Copy link
Contributor Author

The most recent commit should address the requested more data improvement.

@jonbartels
Copy link
Contributor

I pushed changes to not show the dialog when there are no notifications

jonbartels
jonbartels previously approved these changes May 21, 2025
jonbartels
jonbartels previously approved these changes May 21, 2025
@rogin
Copy link
Contributor

rogin commented May 22, 2025

Brainstorming idea on a commit.

Maybe it's over designing, but I noticed the branding branch and refactored the release URL to BrandingConstants which they created. From there, it makes sense for getNotifications to be an interface so others can read versions from a file or other source. So I wrote a default implementation that will process a Github project URL declared at BrandingConstants.RELEASES_URL, and those that read from a file can specify a different class that implements the interface. Since changing the "Accept" header didn't return the crisp data we were hoping for, I omitted it.

@tonygermano
Copy link
Member

Brainstorming idea on a commit.

Maybe it's over designing, but I noticed the branding branch and refactored the release URL to BrandingConstants which they created. From there, it makes sense for getNotifications to be an interface so others can read versions from a file or other source. So I wrote a default implementation that will process a Github project URL declared at BrandingConstants.RELEASES_URL, and those that read from a file can specify a different class that implements the interface. Since changing the "Accept" header didn't return the crisp data we were hoping for, I omitted it.

I figured it was out of scope for this PR, but at some point I'd like to make a plugin interface for notification suppliers, and this could become a reference implementation. I know there have been requests in the past for people to be able to create their own notifications to send to their users and may prefer a notification source other than github releases. I didn't want those ideas to slow progress on this PR, which I think does a fine job of filling a short-term need.

jonbartels
jonbartels previously approved these changes May 22, 2025
@jonbartels jonbartels mentioned this pull request May 27, 2025
@kpalang kpalang self-requested a review May 27, 2025 20:17
@kpalang
Copy link
Contributor

kpalang commented May 27, 2025

The implementation looks quite impressive and massive kudos to @kelaompachai for building it! But there's one but I'm having a hard time overlooking.
The current approach is very GitHub API-centric. So-much-so that any other server would require substantial rewrite. I feel bad bringing this out at such a late stage of the PR, but there were mentions under #24 of serving notifications via RSS or Atom, which in my opinion would be a more flexible approach to delivering notifications. This way any downstreams would simply have to provide a feed url rather than rewrite the fetching and rendering logic.

Another thing I see little value in is having the notifications version-specific. In my opinion we don't really have that much notifications and info specific to a single version that this more useful than it is complex.

- Added library java-semver-0.10.2.jar for version parsing and comparison.
- Refactored to use streams and added tests
- No longer calls a self-hosted service, so active extensions are no
  longer provided as part of the request.

Co-authored-by: Richard Ogin <rogin@users.noreply.github.com>
Co-authored-by: Jon Bartels <jon.bartels@teladochealth.com>
Co-authored-by: Tony Germano <tony@germano.name>
Signed-off-by: kelaompachai <141376761+kelaompachai@users.noreply.github.com>
Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>
Signed-off-by: Tony Germano <tony@germano.name>
Issue: #24
@tonygermano
Copy link
Member

I rebased this PR, made some changes, and then squashed it.

@tonygermano tonygermano linked an issue Jun 3, 2025 that may be closed by this pull request
@jonbartels jonbartels requested review from a team, gibson9583, pacmano1 and ssrowe and removed request for a team June 3, 2025 13:01
Copy link
Contributor

@gibson9583 gibson9583 left a comment

Choose a reason for hiding this comment

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

LGTM

@tonygermano tonygermano merged commit 59889f6 into OpenIntegrationEngine:main Jun 3, 2025
2 checks passed
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 sample notifications server

7 participants