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

Date sorting now works #174

Merged
merged 4 commits into from
Nov 7, 2024
Merged

Date sorting now works #174

merged 4 commits into from
Nov 7, 2024

Conversation

sckott
Copy link
Collaborator

@sckott sckott commented Nov 1, 2024

fix #173

@sckott sckott added the v1.2 Should be implemented in PROOF v1.2 label Nov 1, 2024
@sckott sckott requested a review from tefirman November 1, 2024 22:41
Copy link
Collaborator

@tefirman tefirman left a comment

Choose a reason for hiding this comment

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

Code changes look good, just had one clarifying question. Merging to dev and testing in proof-dev.fredhutch.org, will report back. Thanks @sckott !!

dat <- Filter(\(w) {
w$data$status %in% input$workStatus
}, dat)
}
# Filter by workflow name
if (nzchar(input$workName)) {
dat <- Filter(\(w) {
w$data$workflow_name == input$workName
w$data$workflow_name %in% input$workName
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I understanding right that this addresses #112?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. The %in% operator just changes behavior so that input$workName can be a vector instead of a string with ==. It works for now, but it's not needed for the current input$workName, because the input is a single string. I think this is leftover from when I was testing the many inputs style box where the user could put in > 1 workflow name. I'll undo this change to make it more clear

@tefirman tefirman merged commit 75c8814 into dev Nov 7, 2024
1 check passed
@tefirman
Copy link
Collaborator

tefirman commented Nov 7, 2024

@sckott -- Date sorting works great! It appears that we're now limiting people to 4 months of job history, which isn't necessarily a bad thing, just curious of the rationale. Is that for app responsiveness purposes? Wondering if (in the future, not now) we can allow for longer date ranges, but just default to 4 months.

Also with respect to my earlier comment, if that line was intended to address #112, it doesn't appear to be working, but I might be misreading the purpose of that code change..

@sckott
Copy link
Collaborator Author

sckott commented Nov 8, 2024

limiting people to 4 months of job history

Before this PR we were at two months. And before this switchover to bsilb Amy had set it at a max of I think 21 days. Anywho, yeah I think more history is better too. Of course we I'm sure would run into performance issues with the app if there was too mjuch data - I don't know how much too much would be. Should I try 1 year and see how that goes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.2 Should be implemented in PROOF v1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants