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

[NEW] Rocket.cat message for users when an app previously requested is installed #27672

Merged
merged 22 commits into from Jan 11, 2023

Conversation

matheuslc
Copy link
Contributor

@matheuslc matheuslc commented Dec 30, 2022

Proposed changes (including videos or screenshots)

This PR adds a new cronjob to notify end-users that an app they requested is now enabled. The cronjob is scheduled to run every 24 hours after the workspace started. I'm using this strategy to avoid many workspaces hitting marketplace-api at the same time.

Also, worth to mention, this long polling strategy was chosen as we can have many users waiting for notifications, so it's a process-heavy task that it's better to do in the background.

end-users-pr@2x

Issue(s)

Task

Steps to test or reproduce

Right now, it's only possible to add app requests through the marketplace API. So, we need to wait for the final version of apps requests to land in staging to staging testing properly.

Further comments

@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Merging #27672 (07c68fb) into develop (4271efd) will increase coverage by 0.78%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #27672      +/-   ##
===========================================
+ Coverage    41.46%   42.24%   +0.78%     
===========================================
  Files          841      814      -27     
  Lines        17701    17189     -512     
  Branches      1996     1921      -75     
===========================================
- Hits          7339     7262      -77     
+ Misses       10119     9687     -432     
+ Partials       243      240       -3     
Flag Coverage Δ
e2e 42.24% <0.00%> (+0.78%) ⬆️

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

Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Great work! I'd like to see this moved to 12 hours instead of 24 hours, that way there isn't too long of a delay.

apps/meteor/app/apps/server/communication/rest.js Outdated Show resolved Hide resolved
apps/meteor/app/apps/server/appRequestsCron.ts Outdated Show resolved Hide resolved
apps/meteor/app/apps/server/appRequestsCron.ts Outdated Show resolved Hide resolved
@graywolf336 graywolf336 marked this pull request as ready for review January 6, 2023 19:48
@graywolf336 graywolf336 requested review from a team as code owners January 6, 2023 19:48
graywolf336
graywolf336 previously approved these changes Jan 6, 2023
Copy link
Contributor

@graywolf336 graywolf336 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 from the Marketplace perspective.

apps/meteor/server/lib/sendDirectMessageToUsers.ts Outdated Show resolved Hide resolved
apps/meteor/client/views/admin/apps/AppMenu.js Outdated Show resolved Hide resolved
@graywolf336 graywolf336 added this to the 6.0.0 milestone Jan 11, 2023
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jan 11, 2023
@kodiakhq kodiakhq bot merged commit 029af52 into develop Jan 11, 2023
@kodiakhq kodiakhq bot deleted the feat/install-app-request-notification branch January 11, 2023 18:41
@sampaiodiego sampaiodiego mentioned this pull request Feb 17, 2023
@sampaiodiego sampaiodiego mentioned this pull request Mar 9, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad: apps ecosystem AECO stat: QA tested stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants