-
Notifications
You must be signed in to change notification settings - Fork 482
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
Link enrollment to application in workshop view #33615
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c1a26cd
Find and show matched application id for an enrollment
hacodeorg 0990aef
Add real URL to application detail view
hacodeorg 6348ac3
Add test for mapping application to enrollment
hacodeorg 1a6559d
Fix UI link and text. Fix serializer unit test.
hacodeorg 6780391
Use button instead of hyperlink
hacodeorg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
dashboard/app/serializers/api/v1/pd/workshop_enrollment_serializer.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you can't search on user id and application id together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because
pd_application_id
is not a column in our database, but a serialized property inside the JSONproperties
column for this application record. We can probably use MySQL 5 JSON features for this, but ActiveRecord won't do it for us.I was wondering if you can use
Pd::Application::TeacherApplicationBase
here, which providespd_workshop_id
so you don't need thetry
below. I'm curious to see if/how that modifies the generated query here.A bigger perf concern, for me, is that using this method and the enrollment serializer is introducing a query-per-enrollment for a workshop page load, which might be a performance issue for very large workshops. We probably don't need to overthink this yet, but let's keep an eye on this feature - if the workshop detail view noticeably slows down, we probably want to pull this logic out to the controller and look up all applications for all enrollments in one query. (Or figure out a change to our data model that makes this less painful.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, thinking on this a little more, it does feel like
pd_application_id
really ought to be a column on enrollment, not a serialized property, with an ActiveRecord association defined.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To play devil's advocate, the only time we care about associating an enrollment with an application is in the case of a teacher enrolling in a 5-day (CSP/D) summer workshop, right? That's a really small (albeit important) fraction of the overall number of enrollments, so it'd be like 95% null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brad's explanation is correct.
pd_application_id
is a property in property bag, which is saved as a string data type in pd_applications table.Using
TeacherApplicationBase
instead ofApplicationBase
will not search the entire pd_applications table. It will default to type = TeacherApplicationBase. pd_applications is 1 table shared by multiple models (Single Table Inheritance).I'll add a JIRA task with potential workarounds Brad and I discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bencodeorg: Agree, the number of of enrollments can be mapped to applications is small. The problem is when you open a workshop view, it will grab all enrollments for that workshop in 1 query. However, for each enrollment, we then make 1 more query to pd_applications table to get matching application_id (no matter if it exists or not). If there is 100 enrollments in a workshop, it will create a total of 101 queries instead of 1. This could be a potential performance issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jira task created: https://codedotorg.atlassian.net/browse/PLC-809