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

Finally implement row review #146

Merged
merged 92 commits into from
Jan 15, 2025
Merged

Finally implement row review #146

merged 92 commits into from
Jan 15, 2025

Conversation

jthompson-arcus
Copy link
Collaborator

Closes #99

@LDSamson
Copy link
Collaborator

@jthompson-arcus Maybe good to discuss the conceptual UI implementation together.

@jthompson-arcus
Copy link
Collaborator Author

@jthompson-arcus Maybe good to discuss the conceptual UI implementation together.

I am happy for all input on the UI implementation. There are several ways it could go and no clear best option.

@jthompson-arcus jthompson-arcus changed the base branch from dev to jt-99-review_by_row-devex December 2, 2024 19:11
@jthompson-arcus jthompson-arcus changed the base branch from jt-99-review_by_row-devex to dev December 2, 2024 19:12
@jthompson-arcus jthompson-arcus marked this pull request as ready for review December 31, 2024 18:28
R/mod_study_forms.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@LDSamson LDSamson left a comment

Choose a reason for hiding this comment

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

In my browser logs, it shows that the tables with the approriate table id's cannot be found. Maybe something is rendering too eagerly?
image

R/mod_review_form_tbl.R Outdated Show resolved Hide resolved
@jthompson-arcus
Copy link
Collaborator Author

jthompson-arcus commented Jan 10, 2025

In my browser logs, it shows that the tables with the approriate table id's cannot be found. Maybe something is rendering too eagerly? image

It's kind of the opposite. It's because the datatables are not rendered yet.

@LDSamson this was hard to fix earlier in the process, but with how the code is currently setup we can handle it easier on the server side.

Comment on lines +128 to +137
observeEvent(show_all(), {
req(table_data(), datatable_rendered())
reload_data(reload_data() + 1)
index <- match("subject_id", colnames(table_data())) - 1
if (show_all()) {
DT::showCols(table_proxy, index)
} else {
DT::hideCols(table_proxy, index)
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just hide the o_reviewed column when show_all is on? I get that you want to implement the feature to review data of all subjects in one form, but right now its not functional and hiding the check boxes feels more natural to me. Maybe then you can even remove the disabled column/logic for now (you can always reintroduce it when its needed for reviewing all subjects in a form).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In what way is it not functional right now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also do this in a follow-up PR if you think this is a good idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but unless I know what's not functioning, it will be pretty hard to fix.

Copy link
Collaborator

@LDSamson LDSamson Jan 13, 2025

Choose a reason for hiding this comment

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

With 'not functional' I meant the enhancement mentioned in #22 (reviewing data of all subjects at the same time in a form) is not yet functional. This question was not about fixing anything, but more about just hiding the review column when viewing all subjects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it.

I think we should leave the column visible because the checkboxes are disabled for all subjects not currently being reviewed. The current functionality is consistent with how reviews worked previously.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm do you have a specific reason to leave the column visible? Are there specific benefits that I am missing? I will try to explain. For me personally, having the check boxes visible when viewing all subjects feels a bit awkward since viewing all subjects is about viewing general trends, not about reviewing one subject's data. Hiding the check boxes feels more natural in such a state. Furthermore, the disabled state is a bit hard to see, and it is not immediately clear at a first glance as to why they are disabled, which can cause confusion.

@aclark02-arcus what do you think here?
Again, we can discuss later as well, it is not a deal breaker since it all works, it's more about optimization/user experience. I can also ask some feedback from users internally first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do see @LDSamson's point. Users probably wouldn't consider reviewing the selected patient while viewing all patients. So from that perspective, I see why hiding the Review Status column would cause less confusion. However, I could go either way with this one.

One thing that is WAY more confusing (to me) when expanding to show all patients is that all patient data (1) doesn't show the existing patient first and (2) the rows aren't sorted by patient. Everything is just kind of "jumbled". Sorry, I know this isn't really related to the topic at hand. 🤷🏼‍♂️

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couple of comments:

  • We do not disable the overall review button. In my opinion, if we want to remove row review, we need to disable reviews period. Otherwise it gets way too tricky. For example, if a user clicked on a couple rows and then wanted to check some data against the population, what should we do? Remove his checks? Allow him to click submit review on the partial review they initiated? Feels like we would needlessly be complicating this process and opening it to bugs.
  • Currently the active review is being maintained when a user clicks "Show all participants". I think this is the most desirable outcome. If we were not to preserve the active review, we would at least need to display a modal letting the user know the effects from clicking the toggle.

We can of course discuss this further, but I think a change like this should be a different PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not change the active_review_state (better name for o_reviewed?), which I did not propose to do here and which I think does not happen after hiding the review column. Everything will be as before after switching back to only showing one patient. I agree that that is the most desirable.

Yes, we can also disable the overall review button for consistency. And yes, I think this can be a different PR as I stated before.

@LDSamson
Copy link
Collaborator

Things to consider for a change, but can be a follow up PR:

  • It would be great if the check boxes are hidden if a form is marked as not required to review. Maybe by passing the forms_to_review_data from the app_server to the r object
  • think about splitting o_reviewed data from the table in shiny (and just combining them in the datatable object). I think this would help with the logic and make the reactive flow more natural (although I would have to try it out a bit to confirm this).

@LDSamson
Copy link
Collaborator

@aclark02-arcus do you have any comments on this PR?

@aclark02-arcus
Copy link
Collaborator

@aclark02-arcus do you have any comments on this PR?

Yes, it's beautiful! @LDSamson, we've been using this internally now for a while... well, a version of this PR, before its the review function was modularized which is a really nice touch. Huge improvement! It has my blessing.

Merge branch 'dev' into jt-99-review_by_row_for_reals

# Conflicts:
#	R/mod_common_forms.R
#	R/mod_study_forms.R
#	tests/testthat/_snaps/app_feature_01/app-feature-1-002.json
#	tests/testthat/_snaps/app_feature_01/app-feature-1-002_.png
#	tests/testthat/_snaps/app_feature_01/app-feature-1-003.json
#	tests/testthat/_snaps/app_feature_01/app-feature-1-004.json
#	tests/testthat/_snaps/app_feature_01/app-feature-1-004_.png
#	tests/testthat/_snaps/app_feature_01/app-feature-1-005.json
#	tests/testthat/_snaps/app_feature_01/app-feature-1-005_.png
#	tests/testthat/_snaps/app_feature_03/app-feature-3-002.json
#	tests/testthat/_snaps/mod_study_forms/study_forms-001.json
#	tests/testthat/_snaps/mod_study_forms/study_forms-002.json
Copy link
Collaborator

@LDSamson LDSamson left a comment

Choose a reason for hiding this comment

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

Thanks for adding the comments in the javascript section.

I think we should proceed with merging this PR. There are some things that can be optimized (see all the comments), but that can be done in future PRs.

One thing that is in my opinion most important after merging this, is creating new unit tests/ end to end tests to capture the new behavior. All major features should be tested (ideally using the same BDD test descriptions as used before) before we can use the version in production.

@LDSamson LDSamson merged commit e91ab29 into dev Jan 15, 2025
5 checks passed
@LDSamson LDSamson deleted the jt-99-review_by_row_for_reals branch January 15, 2025 16:17
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.

3 participants