-
Notifications
You must be signed in to change notification settings - Fork 4
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
Resolve question finder default context race conditions #1131
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1131 +/- ##
==========================================
+ Coverage 36.50% 36.53% +0.02%
==========================================
Files 429 432 +3
Lines 19120 19222 +102
Branches 5653 5667 +14
==========================================
+ Hits 6980 7023 +43
- Misses 12102 12161 +59
Partials 38 38 ☔ View full report in Codecov by Sentry. |
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.
It's good that you've found a solution.
What we are aiming for is to set the filter options whenever the user, or maybe its user context, changes. Rather than adding an extra useState to get around the problem, do we know why that intended behaviour is not being achieved by the populateFromUserContext useEffect? Happy to chat through this as it is quite a tricky bit of code!
@mlt47 Digging a little bit deeper, I've found that the crux of the issue seems to be It seems with some quick testing like it can be solved fairly simply by separating |
This now boiled down to two main changes: This is needed so an initial 'default' user state doesn't take precedence over the real user context, but URL parameters do in order to allow the page to be refreshed/copied/etc. 2 - Making the This is needed so |
The previous code set the stage and exam board settings from the user's viewing context. Although the naming is a little complicated, the user's viewing context is not the same as the user's account level registered contexts. It is those account level registered contexts that we are interested in for pre-populating filters. If the user comes to the question finder with query params specifying the stages and exambaords filter settings they take presedence over account settings.
@mlt47 I think your change makes a lot of sense. Only once a However, I think this solution fails to cover some of the edge cases:
|
const [populatedFromAccountSettings, setPopulatedFromAccountSettings] = useState(false); | ||
useEffect(function populateFiltersFromAccountSettings() { | ||
if (isLoggedIn(user)) { | ||
const filtersHaveNotBeenSpecifiedByQueryParams = !params.stages && !params.examBoards; |
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 this is the only blocking change. Anything else like removing the meaningless "all" tag can be made into its own card, since I think we're deciding that having no filters applied when logged out is fine.
const filtersHaveNotBeenSpecifiedByQueryParams = !params.stages && !params.examBoards; | |
const filtersHaveNotBeenSpecifiedByQueryParams = Object.keys(params).every(param => ["stage","examBoard"].includes(param) || !params[param]); |
There are probably other ways to handle this line, but this seems clean to me.
Fixes some issues with users sometimes being fully- or partially-locked into default user context rather than their own preferences when loading the Question Finder (hence showing either all or no questions, rather than their custom filters).
Also changes the default text for if no filters have been applied on first loading the page.
No results match your criteria
->Please select and apply filters
To recreate the issue, load https://adacomputerscience.org/questions with no URL parameters and
My Account > Customize > Show me content for... [GCSE] [AQA]
(or some other non-default combination).
On my end, I was getting ALL questions on first page load, then NO questions upon refreshing that page, followed by the correct filters applying every subsequent refresh (although I wouldn't be surprised if this was device-specific).
To fix this, we check if the filters were previously based on the default context (
userContextDefault
), and reset them if we now have a non-default context.