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

Mer 2788 remove unbounded serial db reads from practice survey views #4536

Conversation

tomasferok
Copy link
Contributor

@tomasferok tomasferok commented Dec 22, 2023

MER-2788
-This pull request enhances the efficiency of the get_preview_render function in surveys and practice activities by extracting activity details. The improvement lies in optimizing the enum map. Additionally, the get_activities function has been refactored to selectively choose only the necessary labels

Observations:

  • Several queries exist within the functions in the enum map. While they seem to follow a consistent logic, I attempted to refactor some of them, encountering complexity in certain cases. Please review those instances.
  • The functions containing queries are currently housed in a liveview module. Would it be beneficial to separate them into a context module?
  • Certain private functions seem to encapsulate logic that could be tested. Consider decoupling these functions and introducing tests for the underlying logic.
    I propose breaking down these tasks into separate tickets for better organization and tracking, how about it?.

Copy link
Contributor

@simonchoxx simonchoxx left a comment

Choose a reason for hiding this comment

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

I think the changes for a first improvement are fine 👏

Maybe we could clean up the get_activities function also in the practice_activities module, as it was done in the surveys module.

I don't understand why a file called logfile was added in the commits.

@tomasferok tomasferok force-pushed the MER-2788-Remove-unbounded-serial-DB-reads-from-Practice-Survey-views branch 2 times, most recently from f09b360 to efdf61c Compare December 28, 2023 15:34
Copy link
Contributor

@darrensiegel darrensiegel left a comment

Choose a reason for hiding this comment

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

This isn't yet where it needs to be, as there is still an unbounded, data-driven serial DB read issue. Each of add_single_response_details, add_choices_frequencies, add_multi_input_details, and add_likert_details all issue DB reads. These functions are called within an Enum.map over all activity attempts.

Instead, replace the query for ActivityAttempt records found in get_activities_details with one that fetches all V2 analytics records (i.e., ResourceSummary, StudentResponse, ResourcePartResponse) for the activities found on that page. Then post process to arrange the data in a way that can be displayed on a per-activity basis. I do not believe that you need to involve ActivityAttempt or ResourceAttempt records in any place here. It is sufficient to simply query for activity resource ids which are found on this page id from the ResourceSummary. In a separate query you can resolve the latest Revisions of those activities that you find.

@darrensiegel
Copy link
Contributor

Also, please retarget this to the hotfix-v0.26.1 branch

@tomasferok tomasferok changed the base branch from master to hotfix-v0.26.1 January 15, 2024 20:20
@tomasferok tomasferok force-pushed the MER-2788-Remove-unbounded-serial-DB-reads-from-Practice-Survey-views branch from fa9a761 to 7a9516e Compare January 15, 2024 20:45
@tomasferok tomasferok force-pushed the MER-2788-Remove-unbounded-serial-DB-reads-from-Practice-Survey-views branch from c7c181d to 275ed4b Compare January 17, 2024 14:50
@darrensiegel darrensiegel merged commit e36009c into hotfix-v0.26.1 Jan 19, 2024
5 checks passed
@darrensiegel darrensiegel deleted the MER-2788-Remove-unbounded-serial-DB-reads-from-Practice-Survey-views branch January 19, 2024 13:09
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