Skip to content

update session details with latest course information before switchin… #3083

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

update session details with latest course information before switchin… #3083

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4e3749
update session details with latest course information before switchin…
C-likethis123 Jan 30, 2025
dd07d9a
Merge branch 'master' into fix-switching-courses-bug
RichDom2185 Feb 20, 2025
cc784c2
Merge branch 'master' into fix-switching-courses-bug
RichDom2185 Mar 31, 2025
69d933f
Merge branch 'master' into fix-switching-courses-bug
RichDom2185 Mar 31, 2025
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
17 changes: 14 additions & 3 deletions src/commons/dropdown/DropdownCourses.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { Dialog, DialogBody, HTMLSelect } from '@blueprintjs/core';
import { IconNames } from '@blueprintjs/icons';
import React from 'react';
import React, { useEffect } from 'react';
import { useDispatch } from 'react-redux';
import { useNavigate } from 'react-router';

import SessionActions from '../application/actions/SessionActions';
import { Role } from '../application/ApplicationTypes';
import { UserCourse } from '../application/types/SessionTypes';
import { useTypedSelector } from '../utils/Hooks';

type Props = {
isOpen: boolean;
Expand All @@ -15,18 +18,26 @@ type Props = {

const DropdownCourses: React.FC<Props> = ({ isOpen, onClose, courses, courseId }) => {
const navigate = useNavigate();
const dispatch = useDispatch();
const latestCourse = useTypedSelector(state => state.session.courseId);

const options = courses.map(course => ({
value: course.courseId,
label: course.courseName.concat(!course.viewable ? ' - disabled' : ''),
disabled: !course.viewable && course.role !== Role.Admin
}));

const onChangeHandler = (e: React.ChangeEvent<HTMLSelectElement>) => {
navigate(`/courses/${e.currentTarget.value}`);
const onChangeHandler = async (e: React.ChangeEvent<HTMLSelectElement>) => {
await dispatch(SessionActions.updateLatestViewedCourse(Number(e.currentTarget.value)));
Copy link
Member

Choose a reason for hiding this comment

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

image

I don't think you can await a dispatch call? We should be handling the async logic via sagas instead.

What was the idea behind this change?

Copy link
Author

@C-likethis123 C-likethis123 Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @RichDom2185, thank you for the feedback and sorry for the late reply.

I agree that the logic is odd - here's my reasoning behind the change:

  1. when switching between courses, the router uses enableGame from the state of the current course to decide whether to route to the game page, or to route to another page.
  2. this enableGame state is based on the current course, not the course that the user wants to navigate to.
  3. the state is only updated via an API call after the routing logic kicks in and the user finishes navigation to a page.
  4. the idea behind awaiting the dispatch call is to update the state of the latest viewed course to the course that I am trying to navigate to, before the routing logic kicks in

I can try to explore handling the async logic via sagas, but another suggestion from me is to enforce a navigation to a page that is common among all courses (eg the home page) so we don't have to use this conditional logic.

onClose();
};

useEffect(() => {
if (latestCourse) {
navigate(`/courses/${latestCourse}`);
}
}, [latestCourse, navigate]);

return (
<Dialog
icon={IconNames.PROPERTIES}
Expand Down
Loading