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.

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

Include colors and theme when sharing timetable #3467

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
66 changes: 65 additions & 1 deletion website/src/utils/timetables.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
TimetableDayFormat,
} from 'types/timetables';
import { LessonType, RawLesson, Semester, Weeks } from 'types/modules';
import { ModulesMap } from 'types/reducers';
import { ColorMapping, ModulesMap } from 'types/reducers';

import _ from 'lodash';

Expand All @@ -34,6 +34,8 @@ import {
arrangeLessonsForWeek,
arrangeLessonsWithinDay,
deserializeTimetable,
deserializeTimetableColors,
deserializeTimetableThemeId,
doLessonsOverlap,
findExamClashes,
formatNumericWeeks,
Expand Down Expand Up @@ -402,6 +404,68 @@ test('timetable serialization/deserialization', () => {
});
});

test('timetable serialization/deserialization with colors', () => {
const configs: SemTimetableConfig[] = [
leslieyip02 marked this conversation as resolved.
Show resolved Hide resolved
{},
{ CS1010S: {} },
{
GER1000: { Tutorial: 'B01' },
},
{
CS2104: { Lecture: '1', Tutorial: '2' },
CS2105: { Lecture: '1', Tutorial: '1' },
CS2107: { Lecture: '1', Tutorial: '8' },
CS4212: { Lecture: '1', Tutorial: '1' },
CS4243: { Laboratory: '2', Lecture: '1' },
GER1000: { Tutorial: 'B01' },
},
];

const emptyColors: ColorMapping = {};
const colorMapping: ColorMapping = {
CS1010S: 0,
GER1000: 1,
CS2104: 2,
};

configs.forEach((config) => {
// if mod config is not present, no color mapping is serialized
const colors = Object.fromEntries(
Object.entries(colorMapping).filter(([moduleCode]) => moduleCode in config),
);
leslieyip02 marked this conversation as resolved.
Show resolved Hide resolved
// empty color mapping should return null
const expectedColors = _.isEqual(colors, emptyColors) ? null : colors;

expect(deserializeTimetable(serializeTimetable(config, colors))).toEqual(config);
expect(deserializeTimetableColors(serializeTimetable(config, colors))).toEqual(expectedColors);
});
});

test('timetable serialization/deserialization with theme', () => {
const configs: SemTimetableConfig[] = [
{},
{ CS1010S: {} },
{
GER1000: { Tutorial: 'B01' },
},
{
CS2104: { Lecture: '1', Tutorial: '2' },
CS2105: { Lecture: '1', Tutorial: '1' },
CS2107: { Lecture: '1', Tutorial: '8' },
CS4212: { Lecture: '1', Tutorial: '1' },
CS4243: { Laboratory: '2', Lecture: '1' },
GER1000: { Tutorial: 'B01' },
},
];

const themeId = 'monokai';

configs.forEach((config) => {
expect(deserializeTimetable(serializeTimetable(config, null, themeId))).toEqual(config);
expect(deserializeTimetableThemeId(serializeTimetable(config, null, themeId))).toEqual(themeId);
});
});

test('deserializing edge cases', () => {
// Duplicate module code
expect(deserializeTimetable('CS1010S=LEC:01&CS1010S=REC:11')).toEqual({
Expand Down
91 changes: 75 additions & 16 deletions website/src/utils/timetables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {

import {
ColoredLesson,
ColorIndex,
HoverLesson,
Lesson,
ModuleLessonConfig,
Expand All @@ -46,8 +47,9 @@ import {
TimetableDayFormat,
} from 'types/timetables';

import { ModuleCodeMap, ModulesMap } from 'types/reducers';
import { ColorMapping, ModuleCodeMap, ModulesMap } from 'types/reducers';
import { ExamClashes } from 'types/views';
import { ThemeId } from 'types/settings';

import { getTimeAsDate } from './timify';
import { getModuleSemesterData, getModuleTimetable } from './modules';
Expand All @@ -72,10 +74,17 @@ export const LESSON_TYPE_ABBREV: lessonTypeAbbrev = {
// Reverse lookup map of LESSON_TYPE_ABBREV
export const LESSON_ABBREV_TYPE: { [key: string]: LessonType } = invert(LESSON_TYPE_ABBREV);

const COLOR_PARAM = 'Color';
const QUERY_PARAM_ABBREV: { [key: string]: string } = {
...LESSON_TYPE_ABBREV,
[COLOR_PARAM]: 'COL',
};
const THEME_PARAM = 'THEME';

// Used for module config serialization - these must be query string safe
// See: https://stackoverflow.com/a/31300627
export const LESSON_TYPE_SEP = ':';
export const LESSON_SEP = ',';
export const QUERY_PARAM_VALUE_SEP = ':';
export const QUERY_PARAM_SEP = ',';

const EMPTY_OBJECT = {};

Expand Down Expand Up @@ -358,20 +367,20 @@ export function getSemesterModules(
return values(pick(modules, Object.keys(timetable)));
}

function serializeModuleConfig(config: ModuleLessonConfig): string {
// eg. { Lecture: 1, Laboratory: 2 } => LEC=1,LAB=2
return map(config, (classNo, lessonType) =>
[LESSON_TYPE_ABBREV[lessonType], encodeURIComponent(classNo)].join(LESSON_TYPE_SEP),
).join(LESSON_SEP);
function serializeModuleConfig(config: { [paramName: string]: string | ColorIndex }): string {
// eg. { Lecture: 1, Laboratory: 2, Color: 3 } => LEC=1,LAB=2,COL=3
return map(config, (paramValue, paramName) =>
[QUERY_PARAM_ABBREV[paramName], encodeURIComponent(paramValue)].join(QUERY_PARAM_VALUE_SEP),
).join(QUERY_PARAM_SEP);
}

function parseModuleConfig(serialized: string | string[] | null): ModuleLessonConfig {
const config: ModuleLessonConfig = {};
if (!serialized) return config;

castArray(serialized).forEach((serializedModule) => {
serializedModule.split(LESSON_SEP).forEach((lesson) => {
const [lessonTypeAbbr, classNo] = lesson.split(LESSON_TYPE_SEP);
serializedModule.split(QUERY_PARAM_SEP).forEach((lesson) => {
const [lessonTypeAbbr, classNo] = lesson.split(QUERY_PARAM_VALUE_SEP);
const lessonType = LESSON_ABBREV_TYPE[lessonTypeAbbr];
// Ignore unparsable/invalid keys
if (!lessonType) return;
Expand Down Expand Up @@ -434,18 +443,68 @@ export function formatNumericWeeks(weeks: NumericWeeks): string | null {
// Converts a timetable config to query string
// eg:
// {
// CS2104: { Lecture: '1', Tutorial: '2' },
// CS2107: { Lecture: '1', Tutorial: '8' },
// CS2104: { Lecture: '1', Tutorial: '2', Color: 3 },
// CS2107: { Lecture: '1', Tutorial: '8', Color: 4 },
// }
// => CS2104=LEC:1,Tut:2&CS2107=LEC:1,Tut:8
export function serializeTimetable(timetable: SemTimetableConfig): string {
// => CS2104=LEC:1,TUT:2,COL:3&CS2107=LEC:1,TUT:8,COL:4
export function serializeTimetable(
timetable: SemTimetableConfig,
colors?: ColorMapping | null,
themeId?: ThemeId | null,
): string {
const timetableModuleParams = Object.fromEntries(
Object.entries(timetable).map((module) => {
const [moduleCode, moduleLessonConfig] = module;
const moduleParams: { [param: string]: string | ColorIndex } = { ...moduleLessonConfig };
if (colors && moduleCode in colors) {
leslieyip02 marked this conversation as resolved.
Show resolved Hide resolved
moduleParams[COLOR_PARAM] = colors[moduleCode];
}
return [moduleCode, moduleParams];
}),
);
const timetableParams = {
...mapValues(timetableModuleParams, serializeModuleConfig),
...((themeId && { [THEME_PARAM]: themeId }) || EMPTY_OBJECT),
};
leslieyip02 marked this conversation as resolved.
Show resolved Hide resolved
// We are using query string safe characters, so this encoding is unnecessary
return qs.stringify(mapValues(timetable, serializeModuleConfig), { encode: false });
return qs.stringify(timetableParams, { encode: false });
}

// Extracts module configs from query string
export function deserializeTimetable(serialized: string): SemTimetableConfig {
Copy link
Member

@ZhangYiJiang ZhangYiJiang Jul 31, 2023

Choose a reason for hiding this comment

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

It's best to rewrite this so that it parses from query string, then delegate to objects to extract individual parts instead of having each functions re-parse the query string.

const params = qs.parse(serialized);
return mapValues(params, parseModuleConfig);
const moduleParams = Object.fromEntries(
Object.entries(params).filter(([paramName]) => paramName !== THEME_PARAM),
);
return mapValues(moduleParams, parseModuleConfig);
}

// Extracts color mapping from query string
export function deserializeTimetableColors(serialized: string): ColorMapping | null {
const params = qs.parse(serialized);
const colorMapping: ColorMapping = {};
Object.keys(params)
.filter((param) => param !== THEME_PARAM)
.forEach((moduleCode) => {
castArray(params[moduleCode]).forEach((serializedModule) => {
if (!serializedModule) return;
serializedModule.split(QUERY_PARAM_SEP).forEach((param) => {
const [paramName, paramValue] = param.split(QUERY_PARAM_VALUE_SEP);
if (paramName === QUERY_PARAM_ABBREV[COLOR_PARAM]) {
colorMapping[moduleCode] = parseInt(paramValue, 10);
Copy link
Member

Choose a reason for hiding this comment

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

Since colours are bound to a sub-param inside each module code, this is better extracted as part of deserializeTimetable instead

}
});
});
});
if (isEqual(colorMapping, EMPTY_OBJECT)) return null;
leslieyip02 marked this conversation as resolved.
Show resolved Hide resolved
return colorMapping;
}

// Extracts themeId from query string
export function deserializeTimetableThemeId(serialized: string): ThemeId | null {
const params = qs.parse(serialized);
if (!params || !(THEME_PARAM in params)) return null;
return params[THEME_PARAM] as ThemeId;
}

export function isSameTimetableConfig(t1: SemTimetableConfig, t2: SemTimetableConfig): boolean {
Expand Down
15 changes: 13 additions & 2 deletions website/src/views/routes/paths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { each, kebabCase } from 'lodash';
import { ModuleTitle, Semester, ModuleCode } from 'types/modules';
import { Venue } from 'types/venues';
import { SemTimetableConfig } from 'types/timetables';
import { ColorMapping } from 'types/reducers';
import { ThemeId } from 'types/settings';
import { serializeTimetable } from 'utils/timetables';
import config from 'config';

Expand All @@ -22,8 +24,17 @@ export function timetablePage(semester: Semester): string {
}

export const TIMETABLE_SHARE = 'share';
export function timetableShare(semester: Semester, timetable: SemTimetableConfig): string {
return `${timetablePage(semester)}/${TIMETABLE_SHARE}?${serializeTimetable(timetable)}`;
export function timetableShare(
semester: Semester,
timetable: SemTimetableConfig,
colors?: ColorMapping | null,
themeId?: ThemeId | null,
): string {
return `${timetablePage(semester)}/${TIMETABLE_SHARE}?${serializeTimetable(
timetable,
colors,
themeId,
)}`;
}

// Timetable path -> Semester
Expand Down
17 changes: 13 additions & 4 deletions website/src/views/timetable/ShareTimetable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import type { QRCodeProps } from 'react-qr-svg';

import type { SemTimetableConfig } from 'types/timetables';
import type { Semester } from 'types/modules';
import type { ColorMapping } from 'types/reducers';
import type { ThemeId } from 'types/settings';

import config from 'config';
import { enableShortUrl } from 'featureFlags';
Expand All @@ -26,6 +28,8 @@ const COPY_FAIL: CopyState = 'COPY_FAIL';
type Props = {
semester: Semester;
timetable: SemTimetableConfig;
colors?: ColorMapping;
themeId?: ThemeId;
};

type State = {
Expand All @@ -34,8 +38,13 @@ type State = {
shortUrl: string | null;
};

function shareUrl(semester: Semester, timetable: SemTimetableConfig): string {
return absolutePath(timetableShare(semester, timetable));
function shareUrl(
semester: Semester,
timetable: SemTimetableConfig,
colors?: ColorMapping | null,
themeId?: ThemeId | null,
leslieyip02 marked this conversation as resolved.
Show resolved Hide resolved
): string {
return absolutePath(timetableShare(semester, timetable, colors, themeId));
}

// So that I don't keep typing 'shortUrl' instead
Expand Down Expand Up @@ -66,8 +75,8 @@ export default class ShareTimetable extends React.PureComponent<Props, State> {
}

loadShortUrl = () => {
const { semester, timetable } = this.props;
const url = shareUrl(semester, timetable);
const { semester, timetable, colors, themeId } = this.props;
const url = shareUrl(semester, timetable, colors, themeId);

// Don't do anything if the long URL has not changed
if (this.url === url) return;
Expand Down
11 changes: 10 additions & 1 deletion website/src/views/timetable/TimetableActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { connect } from 'react-redux';
import { toggleTimetableOrientation, toggleTitleDisplay } from 'actions/theme';
import { Semester } from 'types/modules';
import { SemTimetableConfig } from 'types/timetables';
import { ColorMapping } from 'types/reducers';
import { ThemeId } from 'types/settings';

import { Calendar, Grid, Sidebar, Type } from 'react-feather';
import elements from 'views/elements';
Expand All @@ -17,6 +19,8 @@ import styles from './TimetableActions.scss';
type Props = {
semester: Semester;
timetable: SemTimetableConfig;
colors?: ColorMapping;
themeId?: ThemeId;

isVerticalOrientation: boolean;
toggleTimetableOrientation: () => void;
Expand Down Expand Up @@ -83,7 +87,12 @@ const TimetableActions: React.FC<Props> = (props) => (
<div className={styles.buttonGroup} role="group" aria-label="Timetable exporting">
<ExportMenu semester={props.semester} timetable={props.timetable} />

<ShareTimetable semester={props.semester} timetable={props.timetable} />
<ShareTimetable
semester={props.semester}
timetable={props.timetable}
colors={props.colors}
themeId={props.themeId}
/>
</div>
</div>
);
Expand Down
23 changes: 23 additions & 0 deletions website/src/views/timetable/TimetableContainer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { timetablePage, timetableShare } from 'views/routes/paths';
import { BFS1001, CS1010S, CS3216 } from '__mocks__/modules';
import modulesList from '__mocks__/moduleList.json';

import { ColorMapping } from 'types/reducers';
import { TimetableContainerComponent } from './TimetableContainer';

/**
Expand Down Expand Up @@ -146,6 +147,28 @@ describe(TimetableContainerComponent, () => {
expect(screen.getByRole('button', { name: 'Import' })).toBeInTheDocument();
});

test('should set colors from imported colors if provided', async () => {
const semester = 1;
const importedTimetable = { TRUMP2020: { Lecture: '1' }, CS1010S: { Tutorial: '2' } };
const importedColors: ColorMapping = { TRUMP2020: 3, CS1010S: 5 };
const location = timetableShare(semester, importedTimetable, importedColors);
const { store } = make(location);

const state = store.getState();
expect(state.timetables.colors[semester.toString()]).toEqual(importedColors);
});

test('should set theme from imported theme if there is one', async () => {
const semester = 1;
const importedTimetable = { TRUMP2020: { Lecture: '1' } };
const importedThemeId = 'monokai';
const location = timetableShare(semester, importedTimetable, null, importedThemeId);
const { store } = make(location);

const state = store.getState();
expect(state.theme.id).toEqual(importedThemeId);
});

test('should display saved timetable when there is no imported timetable', () => {
const semester = 1;
const location = timetablePage(semester);
Expand Down
Loading