Skip to content

Commit 1c4eb5a

Browse files
zwliewzwliew
authored
website: ShareTimetable: Shorten URL only when requested (#4210)
1 parent 25870eb commit 1c4eb5a

File tree

2 files changed

+73
-59
lines changed

2 files changed

+73
-59
lines changed

‎website/src/views/timetable/ShareTimetable.test.tsx‎

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,52 +36,61 @@ describe('ShareTimetable', () => {
3636
const openModal = (wrapper: ShallowWrapper) => wrapper.find('button').first().simulate('click');
3737
const closeModal = (wrapper: ShallowWrapper) =>
3838
wrapper.find(Modal).first().props().onRequestClose!({} as any);
39-
40-
const openAndWait = async (wrapper: ShallowWrapper) => {
41-
openModal(wrapper);
42-
39+
const shorten = (wrapper: ShallowWrapper) =>
40+
wrapper.find('button[aria-label="Shorten URL"]').simulate('click');
41+
const shortenAndWait = async (wrapper: ShallowWrapper) => {
42+
shorten(wrapper);
4343
await waitFor(() => {
4444
wrapper.update();
45-
return wrapper.find('input').exists();
45+
return !wrapper.find(LoadingSpinner).exists();
4646
});
4747
};
4848

49-
test('should load short URL when the modal is opened', () => {
49+
test('should load short URL when the shorten button is clicked', async () => {
5050
const wrapper = shallow(
5151
<ShareTimetable semester={1} timetable={timetable} hiddenModules={[]} taModules={{}} />,
5252
);
5353
expect(mockAxios.put).not.toHaveBeenCalled();
5454

5555
openModal(wrapper);
5656
expect(wrapper.find(Modal).exists()).toBe(true);
57+
// Should not call the API until the shorten button is clicked
58+
expect(mockAxios.put).toHaveBeenCalledTimes(0);
59+
shorten(wrapper);
5760
expect(mockAxios.put).toHaveBeenCalledTimes(1);
5861
});
5962

60-
test('should cache short URL from the API', () => {
63+
test('should cache short URL from the API', async () => {
6164
const wrapper = shallow(
6265
<ShareTimetable semester={1} timetable={timetable} hiddenModules={[]} taModules={{}} />,
6366
);
6467

65-
// Open, close and open the modal again
66-
openModal(wrapper);
67-
closeModal(wrapper);
68+
// Open the model, shorten and unshorten again
6869
openModal(wrapper);
70+
await shortenAndWait(wrapper);
71+
expect(mockAxios.put).toHaveBeenCalledTimes(1);
72+
await shortenAndWait(wrapper);
6973

70-
// The second open should not cause a second call
74+
// The second shorten click should not cause a second call
75+
await shortenAndWait(wrapper);
7176
expect(mockAxios.put).toHaveBeenCalledTimes(1);
7277
closeModal(wrapper);
7378

74-
// Changing the timetable should cause opening the modal to trigger another API call
79+
// Changing the timetable should cause the shorten button to trigger another API call
7580
wrapper.setProps({ timetable: { CS3216: { Lecture: '1' } } });
81+
// openModal(wrapper);
7682
expect(mockAxios.put).toHaveBeenCalledTimes(1);
7783
openModal(wrapper);
84+
await shortenAndWait(wrapper);
7885
expect(mockAxios.put).toHaveBeenCalledTimes(2);
86+
shorten(wrapper);
7987
closeModal(wrapper);
8088

8189
// Changing the semester should also trigger another API call
8290
wrapper.setProps({ semester: 2 });
8391
expect(mockAxios.put).toHaveBeenCalledTimes(2);
8492
openModal(wrapper);
93+
await shortenAndWait(wrapper);
8594
expect(mockAxios.put).toHaveBeenCalledTimes(3);
8695
});
8796

@@ -91,6 +100,7 @@ describe('ShareTimetable', () => {
91100
);
92101

93102
openModal(wrapper);
103+
shorten(wrapper);
94104
expect(wrapper.find(LoadingSpinner).exists()).toBe(true);
95105
});
96106

@@ -99,7 +109,8 @@ describe('ShareTimetable', () => {
99109
<ShareTimetable semester={1} timetable={timetable} hiddenModules={[]} taModules={{}} />,
100110
);
101111

102-
await openAndWait(wrapper);
112+
openModal(wrapper);
113+
await shortenAndWait(wrapper);
103114

104115
expect(wrapper.find('input').prop('value')).toEqual(MOCK_SHORTURL);
105116
expect(wrapper.find(Maximize2).exists()).toBe(true);
@@ -111,7 +122,8 @@ describe('ShareTimetable', () => {
111122
<ShareTimetable semester={1} timetable={timetable} hiddenModules={[]} taModules={{}} />,
112123
);
113124

114-
await openAndWait(wrapper);
125+
openModal(wrapper);
126+
await shortenAndWait(wrapper);
115127

116128
expect(wrapper.find('button').at(1).prop('disabled')).toBe(true);
117129
expect(wrapper.find('input').prop('value')).toBeTruthy();
@@ -123,7 +135,8 @@ describe('ShareTimetable', () => {
123135
<ShareTimetable semester={1} timetable={timetable} hiddenModules={[]} taModules={{}} />,
124136
);
125137

126-
await openAndWait(wrapper);
138+
openModal(wrapper);
139+
await shortenAndWait(wrapper);
127140

128141
expect(wrapper.find('button').at(1).prop('disabled')).toBe(true);
129142
expect(wrapper.find('input').prop('value')).toBeTruthy();
@@ -135,7 +148,8 @@ describe('ShareTimetable', () => {
135148
<ShareTimetable semester={1} timetable={timetable} hiddenModules={[]} taModules={{}} />,
136149
);
137150

138-
await openAndWait(wrapper);
151+
openModal(wrapper);
152+
await shortenAndWait(wrapper);
139153

140154
expect(wrapper.find('input').prop('value')).not.toContain('hidden');
141155
});
@@ -151,7 +165,8 @@ describe('ShareTimetable', () => {
151165
/>,
152166
);
153167

154-
await openAndWait(wrapper);
168+
openModal(wrapper);
169+
await shortenAndWait(wrapper);
155170

156171
expect(wrapper.find('input').prop('value')).toContain('hidden=CS1010S,CS1231S');
157172
});
@@ -177,7 +192,8 @@ describe('ShareTimetable', () => {
177192
/>,
178193
);
179194

180-
await openAndWait(wrapper);
195+
openModal(wrapper);
196+
await shortenAndWait(wrapper);
181197

182198
expect(wrapper.find('input').prop('value')).toContain(
183199
'ta=MA1521(TUT:1),CS1010S(TUT:1,LAB:1),CS1231S(TUT:2,TUT:3)',
@@ -189,11 +205,12 @@ describe('ShareTimetable', () => {
189205
<ShareTimetable semester={1} timetable={timetable} hiddenModules={[]} taModules={{}} />,
190206
);
191207

192-
await openAndWait(wrapper);
208+
openModal(wrapper);
209+
await shortenAndWait(wrapper);
193210
expect(wrapper.find('input').prop('value')).toEqual(MOCK_SHORTURL);
194211
expect(wrapper.find(Maximize2).exists()).toBe(true);
195212

196-
wrapper.find('button').at(1).simulate('click');
213+
await shortenAndWait(wrapper);
197214
expect(wrapper.find(Maximize2).exists()).toBe(false);
198215
expect(wrapper.find(Minimize2).exists()).toBe(true);
199216
expect(wrapper.find('input').prop('value')).not.toBe(MOCK_SHORTURL);

‎website/src/views/timetable/ShareTimetable.tsx‎

Lines changed: 36 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ type Props = {
3333
type State = {
3434
isOpen: boolean;
3535
urlCopied: CopyState;
36-
shortUrl: string | null;
36+
shortUrl: string | null | undefined; // undefined = not loaded yet; null = failed to load
3737
fullUrl: string | null;
3838
isFullUrl: boolean;
3939
isLoading: boolean;
@@ -48,12 +48,16 @@ function shareUrl(
4848
return absolutePath(timetableShare(semester, timetable, hiddenModules, taModules));
4949
}
5050

51-
function getToolTipContent(shortUrl: string | null, isFullUrl: boolean, isLoading: boolean) {
51+
function getToolTipContent(
52+
shortUrl: string | null | undefined,
53+
isFullUrl: boolean,
54+
isLoading: boolean,
55+
) {
5256
if (isLoading) {
5357
return 'Shortening link';
5458
}
5559

56-
if (!shortUrl) {
60+
if (shortUrl === null) {
5761
return 'Link shortener temporarily unavailable';
5862
}
5963

@@ -79,7 +83,7 @@ export default class ShareTimetable extends React.PureComponent<Props, State> {
7983
override state: State = {
8084
isOpen: false,
8185
urlCopied: NOT_COPIED,
82-
shortUrl: null,
86+
shortUrl: undefined,
8387
fullUrl: null,
8488
isFullUrl: true,
8589
isLoading: false,
@@ -94,48 +98,35 @@ export default class ShareTimetable extends React.PureComponent<Props, State> {
9498
}
9599
}
96100

97-
loadUrl = () => {
101+
updateFullUrl = () => {
98102
const { semester, timetable, hiddenModules, taModules } = this.props;
99103
const url = shareUrl(semester, timetable, hiddenModules, taModules);
100-
101104
// Don't do anything if the long URL has not changed
102105
if (this.url === url) return;
103-
104-
const showFullUrl = () => this.setState({ fullUrl: url, isFullUrl: true });
105106
this.url = url;
107+
this.setState({ fullUrl: url, shortUrl: undefined, isFullUrl: true });
108+
};
106109

107-
// Only try to retrieve shortUrl if the user is online
108-
if (!navigator.onLine) {
109-
showFullUrl();
110+
tryLoadShortUrl = async () => {
111+
// Don't load short URL if we're currently trying or if we already tried.
112+
if (!this.url || !navigator.onLine || this.state.isLoading || this.state.shortUrl !== undefined)
110113
return;
111-
}
112114

113-
this.setState({ fullUrl: url, shortUrl: null, isFullUrl: true, isLoading: true });
114-
115-
const urlURL = new URL(url);
116-
axios
117-
.put('https://shorten.nusmods.com', {
115+
this.setState({ isLoading: true });
116+
const urlURL = new URL(this.url);
117+
try {
118+
const { data } = await axios.put('https://shorten.nusmods.com', {
118119
longUrl: urlURL.pathname + urlURL.search,
119-
})
120-
.then(({ data }) => {
121-
if (data[SHORT_URL_KEY]) {
122-
this.setState({
123-
shortUrl: data[SHORT_URL_KEY],
124-
isFullUrl: false,
125-
isLoading: false,
126-
});
127-
} else {
128-
this.setState({ isLoading: false });
129-
}
130-
})
131-
// Cannot get short URL - just use long URL instead
132-
.catch(() => {
133-
this.setState({ isLoading: false });
134120
});
121+
this.setState({ shortUrl: data[SHORT_URL_KEY] ?? null, isLoading: false });
122+
} catch {
123+
// Cannot get short URL - just use long URL instead
124+
this.setState({ shortUrl: null, isLoading: false });
125+
}
135126
};
136127

137128
openModal = () => {
138-
this.loadUrl();
129+
this.updateFullUrl();
139130
this.setState({ isOpen: true });
140131
};
141132

@@ -161,14 +152,20 @@ export default class ShareTimetable extends React.PureComponent<Props, State> {
161152
}
162153
};
163154

164-
toggleShortenUrl = () => {
155+
toggleShortenUrl = async () => {
156+
await this.tryLoadShortUrl();
165157
this.setState((prevState) => ({
166-
isFullUrl: !prevState.isFullUrl,
158+
isFullUrl: !(prevState.isFullUrl && prevState.shortUrl !== null),
167159
urlCopied: NOT_COPIED,
168160
}));
169161
};
170162

171-
renderSharing(fullUrl: string, shortUrl: string | null, isFullUrl: boolean, isLoading: boolean) {
163+
renderSharing(
164+
fullUrl: string,
165+
shortUrl: string | null | undefined,
166+
isFullUrl: boolean,
167+
isLoading: boolean,
168+
) {
172169
const { semester } = this.props;
173170
const url = isFullUrl ? fullUrl : shortUrl ?? fullUrl;
174171
const toggleUrlButton = isFullUrl ? <Minimize2 /> : <Maximize2 />;
@@ -189,7 +186,7 @@ export default class ShareTimetable extends React.PureComponent<Props, State> {
189186
type="button"
190187
aria-label="Shorten URL"
191188
onClick={this.toggleShortenUrl}
192-
disabled={!shortUrl}
189+
disabled={shortUrl === null || isLoading}
193190
>
194191
{isLoading ? <LoadingSpinner small white /> : toggleUrlButton}
195192
</button>
@@ -273,8 +270,8 @@ export default class ShareTimetable extends React.PureComponent<Props, State> {
273270
type="button"
274271
className="btn btn-outline-primary btn-svg"
275272
onClick={this.openModal}
276-
onMouseOver={this.loadUrl}
277-
onFocus={this.loadUrl}
273+
onMouseOver={this.updateFullUrl}
274+
onFocus={this.updateFullUrl}
278275
>
279276
<Repeat className="svg svg-small" />
280277
Share/Sync

0 commit comments

Comments
 (0)