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

Run processes in background #731

Draft
wants to merge 25 commits into
base: dev
Choose a base branch
from
Draft

Run processes in background #731

wants to merge 25 commits into from

Conversation

Jeff-Thompson12
Copy link
Collaborator

Addresses #711

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: Patch coverage is 79.53488% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 78.78%. Comparing base (6bdf82d) to head (0db252b).

Files Patch % Lines
R/mod_downloadHandler.R 53.33% 35 Missing ⚠️
R/mod_downloadHandler_utils.R 93.38% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #731      +/-   ##
==========================================
- Coverage   79.20%   78.78%   -0.43%     
==========================================
  Files          33       34       +1     
  Lines        5112     5171      +59     
==========================================
+ Hits         4049     4074      +25     
- Misses       1063     1097      +34     

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

@Jeff-Thompson12
Copy link
Collaborator Author

Looking for some feedback on this PR. Interested in what features you guys think should be added or what already needs to be changed.

Merge branch 'dev' into jt-711-r_bg

# Conflicts:
#	R/mod_downloadHandler.R
@Jeff-Thompson12 Jeff-Thompson12 marked this pull request as ready for review January 16, 2024 19:04
@Jeff-Thompson12 Jeff-Thompson12 requested review from AARON-CLARK and Robert-Krajcik and removed request for AARON-CLARK and Robert-Krajcik January 16, 2024 19:05
Merge branch 'dev' into jt-711-r_bg

# Conflicts:
#	NEWS.md
#	R/mod_downloadHandler.R
Copy link
Collaborator

@AARON-CLARK AARON-CLARK left a comment

Choose a reason for hiding this comment

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

Hi @Jeff-Thompson12,

This is pretty sweet! In my first test, I ran html reports for 3 reports, then 5 reports. When downloading 3 reports, I noticed I was still able to do things in the app, which was not expected. Is that normal?

image

When rendering 5 reports, I was able to download a new package to the db while it was running, which is amazing. However, when it finished, the app crashed and there was an error in the console:

image

Merge branch 'dev' into jt-711-r_bg

# Conflicts:
#	DESCRIPTION
#	NEWS.md
#	R/mod_downloadHandler.R
@Jeff-Thompson12
Copy link
Collaborator Author

@aclark02-arcus something else I forgot to mention is that since downloading a report is now a separate event from the report creation it is possible for the report downloader or another user to change outputs. This could be as low risk as adding comments but could also include actions like deleting a package from the database, which now that I mention it could crash the app.

@aclark02-arcus aclark02-arcus marked this pull request as draft April 9, 2024 15:34
@aclark02-arcus
Copy link
Collaborator

Converting to draft while we consider some alternatives, like grabbing info from database and injecting it into a list for rendering all our reports. If we can strip out the riskmetric layout "things", it may become really efficient too.

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.

4 participants