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.

Sign up for GitHub

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

Jump to bottom

Resolve question finder default context race conditions #1131

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
+19 −18
Open

Resolve question finder default context race conditions #1131

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b2758e1
Prevent getting stuck on default context
sjd210 Sep 17, 2024
ccc3bfa
Prevent another case of being stuck on default
sjd210 Sep 17, 2024
94675ae
Remove extra URL update
sjd210 Sep 17, 2024
50e0119
Add userContextDefault to useEffect parameters
sjd210 Sep 17, 2024
e13c70c
Cleanup extra linebreak
sjd210 Sep 17, 2024
179ad22
Restore missing linebreak
sjd210 Sep 17, 2024
78c00e1
Fix linting
sjd210 Sep 17, 2024
7ffd4fa
Set searchAndUpdateURL to run correctly
sjd210 Sep 26, 2024
75a1bc0
Merge branch 'master' into hotfix/question-finder-page-loading
sjd210 Sep 26, 2024
fc5fc66
Merge branch 'bug/question-finder-loading' into hotfix/question-finde…
sjd210 Sep 26, 2024
82be94c
Update q finder filter from account settings
mlt47 Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions src/app/components/pages/QuestionFinder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ function getInitialQuestionStatuses(params: ListParams): QuestionStatus {
export const QuestionFinder = withRouter(({location}: RouteComponentProps) => {
const dispatch = useAppDispatch();
const user = useAppSelector((state: AppState) => state && state.user);
const userContext = useUserViewingContext();
const params: ListParams = useQueryParams(false);
const history = useHistory();

Expand All @@ -115,29 +114,31 @@ export const QuestionFinder = withRouter(({location}: RouteComponentProps) => {
const [excludeBooks, setExcludeBooks] = useState<boolean>(!!params.excludeBooks);
const [searchDisabled, setSearchDisabled] = useState(true);

const [populatedUserContext, setPopulatedUserContext] = useState(false);

useEffect(function populateFromUserContext() {
if (isAda && isLoggedIn(user) && user.registeredContexts && user.registeredContexts.length > 1) {
setSearchStages([STAGE.ALL]);
setSearchExamBoards([EXAM_BOARD.ALL]);
}
else {
if (!STAGE_NULL_OPTIONS.includes(userContext.stage)) {
setSearchStages(arr => arr.length > 0 ? arr : [userContext.stage]);
}
if (!EXAM_BOARD_NULL_OPTIONS.includes(userContext.examBoard)) {
setSearchExamBoards(arr => arr.length > 0 ? arr : [userContext.examBoard]);
const [populatedFromAccountSettings, setPopulatedFromAccountSettings] = useState(false);
useEffect(function populateFiltersFromAccountSettings() {
if (isLoggedIn(user)) {
const filtersHaveNotBeenSpecifiedByQueryParams = !params.stages && !params.examBoards;
Copy link
Contributor Author

@sjd210 sjd210 Oct 9, 2024 •
edited
Loading

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.

Suggested change
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.

if (filtersHaveNotBeenSpecifiedByQueryParams) {
const accountStages = user.registeredContexts?.map(c => c.stage).filter(s => s) as STAGE[];
if (isPhy || accountStages.length === 1) { // Ada only want to apply stages filter if there is only one
setSearchStages(accountStages);
setPopulatedFromAccountSettings(true);
}
const examBoardStages = user.registeredContexts?.map(c => c.examBoard).filter(e => e) as EXAM_BOARD[];
if (isAda && examBoardStages.length === 1) { // Phy does not have exam boards
setSearchExamBoards(examBoardStages);
setPopulatedFromAccountSettings(true);
}
}
}
setPopulatedUserContext(!!userContext.stage && !!userContext.examBoard);
}, [userContext.stage, userContext.examBoard, user]);
}, [user]);

// this acts as an "on complete load", needed as we can only correctly update the URL once we have the user context *and* React has processed the above setStates
useEffect(() => {
searchAndUpdateURL();
setNoResultsMessage(<em>No results match your criteria</em>);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [populatedUserContext]);
}, [populatedFromAccountSettings]);

const [disableLoadMore, setDisableLoadMore] = useState(false);

Expand Down Expand Up @@ -233,7 +234,7 @@ export const QuestionFinder = withRouter(({location}: RouteComponentProps) => {
!( Object.values(searchStatuses).every(v => v) || Object.values(searchStatuses).every(v => !v) )
);

const [noResultsMessage, setNoResultsMessage] = useState<ReactNode>(<em>No results match your criteria</em>);
const [noResultsMessage, setNoResultsMessage] = useState<ReactNode>(<em>Please select and apply filters</em>);

const applyFilters = () => {
// Have to use a local variable as React won't update state in time
Expand Down