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

Add the ability to emit platform wide notifications #4637

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Oct 11, 2024

Description

Added an admin API that can send platform wide notifications to users that fall into the provided roles.
Added a User model query that retrieves users based on provided roles.

Related Issue(s)

closes #4554

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 17.39130% with 19 lines in your changes missing coverage. Please review.

Project coverage is 78.49%. Comparing base (49321fc) to head (a5c9e04).
Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
forge/routes/api/admin.js 16.66% 15 Missing ⚠️
forge/db/models/User.js 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4637      +/-   ##
==========================================
- Coverage   78.53%   78.49%   -0.04%     
==========================================
  Files         303      304       +1     
  Lines       14398    14493      +95     
  Branches     3285     3308      +23     
==========================================
+ Hits        11307    11377      +70     
- Misses       3091     3116      +25     
Flag Coverage Δ
backend 78.49% <17.39%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Overall, looks good.

One pending question about the 'external url' option - more the ability to provide non-external urls. Not sure how self-hosted admins would know what to do with that.

If there isn't a specific need or example where it will be needed, can I suggest we hide for now to avoid confusion?

URL Link
<template #description>Provide an url where users will be redirected when they click on the notification.</template>
</FormRow>
<ff-checkbox v-model="form.externalUrl" data-el="notification-external-url-toggle">
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this is meant to be used for non-external links. It requires quite a deep knowledge of the UI - something self-hosted users won't have. Is there a specific use-case that requires this capability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was mainly for us to know how to properly format the internal url's. I agree there's no use on having it for self-hosted customers. Would leaving the external url as default and hiding the placeholder for the internal routes suffice?

Copy link
Member

Choose a reason for hiding this comment

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

The default should be external URL, and I don't think we should have the option for it to be anything else. If we wanted to add support for routing internally, then we can sniff the value to decide if its a url or an object - no need to store that otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the external url selector, forcing all urls to be external

@cstns
Copy link
Contributor Author

cstns commented Oct 21, 2024

The only task left is inserting data into the notifications table. I’ve added a basic implementation that loops through and inserts notifications into the database based on the roles query.

However, this approach could become problematic if there are many users or results, as it might cause a table lock on the notifications table and lead to other issues.

Should we stick with this for now?

@knolleary
Copy link
Member

However, this approach could become problematic if there are many users or results, as it might cause a table lock on the notifications table and lead to other issues.

Should we stick with this for now?

I had a quick look at this part this morning. Had a couple thoughts, but need to poke at it a bit more once other unrelated tasks are complete. Will review tomorrow.

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.

Admin UI: Add feature to distribute platform-wide notifications
2 participants