-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
[NEW] Notify admins via rocket.cat when a user requests to use an app #27858
[NEW] Notify admins via rocket.cat when a user requests to use an app #27858
Conversation
…o use the newest marketplace route model
async post() { | ||
const { appId, appName, message } = this.bodyParams; | ||
const workspaceUrl = settings.get('Site_Url'); | ||
const learnMore = `${workspaceUrl}marketplace/explore/info/${appId}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes the Site_Url
will end with a slash and sometimes it won't. I highly recommend ensuring it has one and putting it there if it doesn't. However, my information could be wrong, so I'd like input from others :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graywolf336 we can ensure that using a regex that matches only at the end of the string. What do you think?
It would be something like that:
const example = "http://localhost/"
const regex = new RegExp('\\/$', 'gm')
const result = example.replace(regex, '');
console.log('Result:', result)
This way we ensure the site url doesn't have a / at the end of the url and we can add them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Digging more into the code, I've found out that they assume Site_Url always doesn't have the / at the end 🤔 But we may guarantee that through my suggestion above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd rather error on the side of caution than just making an assumption. However, someone more experienced can provide more context and thoughts.
Codecov Report
@@ Coverage Diff @@
## feat/new-marketplace #27858 +/- ##
========================================================
- Coverage 43.23% 43.18% -0.05%
========================================================
Files 821 821
Lines 17041 17041
Branches 2004 2004
========================================================
- Hits 7367 7359 -8
- Misses 9393 9402 +9
+ Partials 281 280 -1
Flags with carried forward coverage won't be shown. Click here to find out more. |
…success toast message when the app request went successful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing new on the Rocket.Chat front.
…hat into feat/admin-rocket-cat-notification
Proposed changes (including videos or screenshots)
This PR adds a new feature on top of the app requests feature to notify all admins whenever a user requests to use an app. The notification happens via rocket.cat user.
Issue(s)
MKP-197
MKP-224
Steps to test or reproduce
Requirements
Requesting an app
Logged as an end user, access the marketplace, and request an app.
Example:
Notification
As soon as the user sends the request, admins will receive a notification.
Further comments