Pulling in pilots for maker homepage course selection #45156
Conversation
dashboard/app/models/script.rb
Outdated
| @@maker_units ||= visible_units.select(&:is_maker_unit?) | ||
| def self.maker_units(user) | ||
| total_units = visible_units | ||
| total_units += all_scripts.select {|s| s.has_pilot_access?(user)} |
There was a problem hiding this comment.
Do you know if this line will cause any db queries (once all_scripts is cached)? It seems a bit expensive to me to have to iterate through all scripts (but we probably do this elsewhere too).
There was a problem hiding this comment.
Thanks for chatting with me about this- I've made a ticket for our backlog to fix this in all places: https://codedotorg.atlassian.net/browse/LP-2228
| def self.maker_units | ||
| @@maker_units ||= visible_units.select(&:is_maker_unit?) | ||
| def self.maker_units(user) | ||
| return_units = @@maker_units ||= visible_units.select(&:is_maker_unit?) |
There was a problem hiding this comment.
(minor, up to you) Now that I see this, I'm wondering whether this would be clearer as two separate functions maker_units and maker_units_for_user or visible_maker_units and maker_units or something like that.
There was a problem hiding this comment.
I see what you mean here. I think if this were used in more places, the clarity would help/make sense. That said, the function is only called from one place so I think it's fine to keep them combined.
Yes! I just pushed two tests: one where a student is enrolled in a pilot and it displays, and another where a pilot experiment (and even a user_script) exists for a student but the student isn't enrolled in the pilot, so it doesn't display. |
There was a problem hiding this comment.
Looks good!
As advice for the future, I encourage you to write unit tests at the smallest meaningful scope (e.g. maker_units in this change). This makes it clearer what the test is testing and reduces maintenance costs by having the test only need to be changed when the logic for that specific scenario changes.
Thank you for this! This is a good reminder. |
Current Behavior: The maker app populates the most recent script in the maker homepage by pulling in 'visible_units' from script.rb. Because pilot scripts are not "visible", these scripts are excluded.
This PR updates the maker controller to add to that initial pull of scripts a list of all pilots a student is enrolled in. These scripts are then sorted by most recent, and the most recent is displayed as the single course on the maker homepage.
Here is what my screen looked like before the changes (I'm enrolled in the pilot, but the last year of the course displays instead):
And now that I have pulled in pilots, the 2022 pilot shows here:
I tested this by adding myself to the pilot locally and ensuring that the pilot is pulled in accordingly.
Links
Testing story
I added two unit tests for this update: one where the student is enrolled in a pilot and that pilot displays on the homepage instead of the 2019 course, and one where the pilot exists (and the user_script exists too), but the student isn't enrolled in the pilot so the 2019 course displays instead.
Deployment strategy
Follow-up work
https://codedotorg.atlassian.net/browse/LP-2228
Privacy
Security
Caching
PR Checklist: