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

Call API with add and remove buttons, refine loading and error strategies (#181, 191) #196

Merged
merged 21 commits into from
Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2504229
Set up api module; adopt react-async preliminarily
ssciolla Jun 8, 2022
0ae3e05
Replace react-async with react-async-hook; get CSRF token with js-coo…
ssciolla Jun 8, 2022
501c7d1
Move update API call state into ToolCard to enable overlapping actions
ssciolla Jun 8, 2022
50c880b
Use LinearProgress for loading, following accessibility instructions
ssciolla Jun 9, 2022
03f7205
Use canvas_id for key to eliminate warning
ssciolla Jun 9, 2022
8b19208
Log error in updateToolNav; add parentheses around statusText
ssciolla Jun 9, 2022
88ea87d
Add eslint stuff for react-hooks, react-async-hook
ssciolla Jun 9, 2022
6213e2d
Refactor useAsync, useAsyncCallback uses to fix error handling
ssciolla Jun 9, 2022
5d63089
Switch to react-query; use onSuccess options
ssciolla Jun 10, 2022
2c860d0
Create ErrorsDisplay; simplify loading pattern
ssciolla Jun 13, 2022
4d08d3b
Remove margin from ErrorsDisplay; use Box with ErrorsDisplay in Home
ssciolla Jun 13, 2022
487c955
Disable retry and refetch behavior in react-query (for now)
ssciolla Jun 13, 2022
ba3bfc6
Remove comment
ssciolla Jun 13, 2022
05a7155
Add refresh alert; fix error spacing logic in Home
ssciolla Jun 13, 2022
c2c0555
Make a couple minor ordering, formatting changes
ssciolla Jun 13, 2022
2c09eb4
Improve spacing with a feedbackBlock; move searchFilter to be with ot…
ssciolla Jun 17, 2022
80f787a
Reorder, group variables in ToolCard
ssciolla Jun 17, 2022
124b5de
Remove use of wildcard import with api
ssciolla Jun 17, 2022
9f8242a
Remove length check in ErrorsDisplay
ssciolla Jun 17, 2022
cc4fd19
Remove extra newline
ssciolla Jun 17, 2022
b82eab2
Use feedbackBlock pattern to eliminate unnecessary whitespace; remove…
ssciolla Jun 17, 2022
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
3 changes: 2 additions & 1 deletion backend/canvas_app_explorer/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ def list(self, request: Request) -> Response:
available_tools = manager.get_tools_available_in_course()
logger.debug('available_tools: ' + ', '.join([tool.__str__() for tool in available_tools]))
available_tool_ids = [t.id for t in available_tools]
queryset = models.LtiTool.objects.filter(canvas_id__isnull=False, canvas_id__in=available_tool_ids)
queryset = models.LtiTool.objects.filter(canvas_id__isnull=False, canvas_id__in=available_tool_ids)\
.order_by('name')
serializer = serializers.LtiToolWithNavSerializer(
queryset, many=True, context={ 'available_tools': available_tools }
)
Expand Down
5 changes: 4 additions & 1 deletion frontend/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module.exports = {
},
'plugins': [
'react',
'react-hooks',
'@typescript-eslint'
],
'settings': {
Expand All @@ -42,6 +43,8 @@ module.exports = {
'semi': [
'error',
'always'
]
],
'react-hooks/rules-of-hooks': 'error',
'react-hooks/exhaustive-deps': ['warn']
}
};
67 changes: 67 additions & 0 deletions frontend/app/api.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import Cookies from 'js-cookie';

import { Tool } from './interfaces';

const API_BASE = '/api';
const JSON_MIME_TYPE = 'application/json';

const BASE_MUTATION_HEADERS: HeadersInit = {
Accept: JSON_MIME_TYPE,
'Content-Type': JSON_MIME_TYPE,
'X-Requested-With': 'XMLHttpRequest'
};

const getCSRFToken = (): string | undefined => Cookies.get('csrftoken');

const createErrorMessage = async (res: Response): Promise<string> => {
let errorBody;
try {
errorBody = await res.json();
} catch {
console.error('Error body was not JSON.');
errorBody = undefined;
}
return (
'Error occurred! ' +
`Status: ${res.status}` + (res.statusText !== '' ? ` (${res.statusText})` : '') +
(errorBody !== undefined ? '; Body: ' + JSON.stringify(errorBody) : '.')
);
};

async function getTools (): Promise<Tool[]> {
const url = `${API_BASE}/lti_tools/`;
const res = await fetch(url);
ssciolla marked this conversation as resolved.
Show resolved Hide resolved
if (!res.ok) {
console.error(res);
throw new Error(await createErrorMessage(res));
}
const data: Tool[] = await res.json();
return data;
}

interface UpdateToolNavData {
canvasToolId: number
navEnabled: boolean
}

async function updateToolNav (data: UpdateToolNavData): Promise<void> {
const { canvasToolId, navEnabled } = data;
const body = { navigation_enabled: navEnabled };
const url = `${API_BASE}/lti_tools/${canvasToolId}/`;
const requestInit: RequestInit = {
method: 'PUT',
body: JSON.stringify(body),
headers: {
...BASE_MUTATION_HEADERS,
'X-CSRFTOKEN': getCSRFToken() ?? ''
Copy link
Contributor Author

@ssciolla ssciolla Jun 13, 2022

Choose a reason for hiding this comment

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

We could maybe throw an error in getCSRFToken instead, but it seems unlikely that the CSRF token won't be there. And this would still have a clear result, the call would fail with a missing CSRF token message.

}
};
const res = await fetch(url, requestInit);
if (!res.ok) {
console.error(res);
throw new Error(await createErrorMessage(res));
zqian marked this conversation as resolved.
Show resolved Hide resolved
}
return;
}

export { getTools, updateToolNav };
20 changes: 20 additions & 0 deletions frontend/app/components/ErrorsDisplay.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React from 'react';
import { Alert, Grid } from '@mui/material';

interface ErrorsDisplayProps {
errors: Error[]
}

export default function ErrorsDisplay (props: ErrorsDisplayProps) {
return (
props.errors.length === 0
? <></>
zqian marked this conversation as resolved.
Show resolved Hide resolved
: (
<Grid container spacing={1} justifyContent='center' direction='column'>
{props.errors.map((e, i) => (
<Grid key={i} item><Alert severity='error'>{e.message}</Alert></Grid>
))}
</Grid>
)
);
}
72 changes: 50 additions & 22 deletions frontend/app/components/Home.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import React, { useEffect, useState } from 'react';
import { Alert, Grid, Typography } from '@mui/material';
import React, { useState } from 'react';
import { useQuery } from 'react-query';
import { Alert, Box, Grid, LinearProgress, Typography } from '@mui/material';
import { styled } from '@mui/material/styles';

import ErrorsDisplay from './ErrorsDisplay';
import HeaderAppBar from './HeaderAppBar';
import ToolCard from './ToolCard';
import * as api from '../api';
ssciolla marked this conversation as resolved.
Show resolved Hide resolved
import '../css/Home.css';
import { Tool } from '../interfaces';

Expand All @@ -22,34 +25,56 @@ const filterTools = (tools: Tool[], filter: string): Tool[] => {
};

function Home () {
const [tools, setTools] = useState<null | Tool[]>(null);
const [tools, setTools] = useState<undefined | Tool[]>(undefined);
const [searchFilter, setSearchFilter] = useState('');
const [showRefreshAlert, setShowRefreshAlert] = useState<undefined | boolean>(undefined);

useEffect(() => {
const fetchToolData = async () => {
const url = '/api/lti_tools/';
const response = await fetch(url);
const data: Tool[] = await response.json();
// sort data alphabetically by name
data.sort((a, b) => (a.name.toLowerCase() > b.name.toLowerCase())
? 1
: ((b.name.toLowerCase() > a.name.toLowerCase()) ? -1 : 0)
);
setTools(data);
};
fetchToolData();
}, []);
const { isLoading: getToolsLoading, error: getToolsError } = useQuery('getTools', api.getTools, {
onSuccess: (data) => setTools(data)
});
ssciolla marked this conversation as resolved.
Show resolved Hide resolved

const onToolUpdate = (newTool: Tool) => {
/*
Creates new array with newTool replacing its previous version;
Uses function inside setState hook to handle overlapping requests
*/
setTools((oldTools) => {
if (oldTools === undefined) throw Error('Expected tools variable to be defined!');
zqian marked this conversation as resolved.
Show resolved Hide resolved
const newTools = oldTools.map(t => t.canvas_id === newTool.canvas_id ? newTool : t);
return newTools;
});

if (showRefreshAlert === undefined) setShowRefreshAlert(true);
};

const isLoading = getToolsLoading;
const errors = [getToolsError].filter(e => e !== null) as Error[];

let feedbackBlock;
if (isLoading || errors.length > 0 || showRefreshAlert) {
feedbackBlock = (
<Box sx={{ margin: 2 }}>
{isLoading && <LinearProgress id='tool-card-container-loading' sx={{ marginBottom: 2 }} />}
{errors.length > 0 && <Box sx={{ marginBottom: 1 }}><ErrorsDisplay errors={errors} /></Box>}
{showRefreshAlert && (
<Alert severity='success' sx={{ marginBottom: 1 }} onClose={() => setShowRefreshAlert(false)}>
Refresh the page to make tool changes appear in the left-hand navigation.
</Alert>
)}
</Box>
);
}

let toolCardContainer;
if (tools === null) {
toolCardContainer = (<div>Loading . . . </div>);
} else {
if (tools !== undefined) {
const filteredTools = searchFilter !== '' ? filterTools(tools, searchFilter) : tools;
toolCardContainer = (
<Grid container spacing={2} justifyContent='center'>
{
filteredTools.length > 0
? filteredTools.map(t => <Grid item key={t.id}><ToolCard tool={t} /></Grid>)
? filteredTools.map(t => (
<Grid item key={t.canvas_id}><ToolCard tool={t} onToolUpdate={onToolUpdate} /></Grid>
))
: <Grid item><Alert severity='info'>No matching results</Alert></Grid>
}
</Grid>
Expand All @@ -63,7 +88,10 @@ function Home () {
<Typography variant='h6' component='h2' sx={{ textAlign: 'center', marginBottom: 3 }}>
Find the best tools for your class and students
</Typography>
{toolCardContainer}
{feedbackBlock}
<div aria-describedby='tool-card-container-loading' aria-busy={getToolsLoading}>
{toolCardContainer}
</div>
</MainContainer>
<Typography component='footer' sx={{ textAlign: 'center' }}>
Copyright © 2022 The Regents of the University of Michigan
Expand Down
47 changes: 43 additions & 4 deletions frontend/app/components/ToolCard.tsx
Original file line number Diff line number Diff line change
@@ -1,28 +1,44 @@
import React, { useState } from 'react';
import AddBox from '@mui/icons-material/AddBox';
import { useMutation } from 'react-query';
import ExpandLessIcon from '@mui/icons-material/ExpandLess';
import ExpandMoreIcon from '@mui/icons-material/ExpandMore';
import {
Button, Card, CardActions, CardContent, CardMedia, Collapse, Grid, Typography
Button, Card, CardActions, CardContent, CardMedia, Collapse, Grid, LinearProgress, Typography
} from '@mui/material';

import DataElement from './DataElement';
import ErrorsDisplay from './ErrorsDisplay';
import ImageDialog from './ImageDialog';
import { AddToolButton, RemoveToolButton } from './toolButtons';
import * as api from '../api';
import { Tool } from '../interfaces';

interface ToolCardProps {
tool: Tool
onToolUpdate: (tool: Tool) => void;
}

export default function ToolCard (props: ToolCardProps) {
const { tool } = props;
const { tool, onToolUpdate } = props;

const [showMoreInfo, setShowMoreInfo] = useState(false);
const [screenshotDialogOpen, setScreenshotDialogOpen] = useState(false);

const moreOrLessText = !showMoreInfo ? 'More' : 'Less';

const {
mutate: doUpdateToolNav, error: updateToolNavError, isLoading: updateToolNavLoading
} = useMutation(api.updateToolNav, { onSuccess: (data, variables) => {
const newTool = { ...tool, navigation_enabled: variables.navEnabled };
onToolUpdate(newTool);
}});

const isLoading = updateToolNavLoading;
const loadingBlock = isLoading && <LinearProgress sx={{ margin: 2 }} id='add-remove-tool-button-loading' />;

const errors = [updateToolNavError].filter(e => e !== null) as Error[];

let mainImageBlock;
if (tool.main_image !== null) {
const defaultMainImageAltText = `Image of ${tool.name} tool in use`;
Expand Down Expand Up @@ -67,9 +83,32 @@ export default function ToolCard (props: ToolCardProps) {
<span dangerouslySetInnerHTML={{ __html: tool.short_description }} />
</Typography>
</CardContent>
<CardContent>
{loadingBlock}
<ErrorsDisplay errors={errors} />
</CardContent>
<CardActions>
<Grid container justifyContent='space-between'>
{tool.navigation_enabled ? <RemoveToolButton /> : <AddToolButton />}
<Grid
container
justifyContent='space-between'
aria-describedby='add-remove-tool-button-loading'
aria-busy={updateToolNavLoading}
>
{
tool.navigation_enabled
? (
<RemoveToolButton
disabled={updateToolNavLoading}
onClick={() => doUpdateToolNav({ canvasToolId: tool.canvas_id, navEnabled: false })}
/>
)
: (
<AddToolButton
disabled={updateToolNavLoading}
onClick={() => doUpdateToolNav({ canvasToolId: tool.canvas_id, navEnabled: true })}
/>
)
}
<Button
onClick={() => setShowMoreInfo(!showMoreInfo)}
aria-expanded={showMoreInfo}
Expand Down
20 changes: 17 additions & 3 deletions frontend/app/index.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,29 @@
import React from 'react';
import ReactDOM from 'react-dom';
import { QueryClient, QueryClientProvider } from 'react-query';
import { ThemeProvider } from '@mui/material';

import Home from './components/Home';
import theme from './theme';

const queryClient = new QueryClient({
defaultOptions: {
queries: {
retry: false,
retryOnMount: false,
staleTime: Infinity
},
mutations: { retry: false }
}
Comment on lines +10 to +17
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-query includes a lot of retry and refetch behavior by default. We may want some of this eventually, but for now I thought it seemed unnecessary and easier for testing to disable it all. See https://react-query.tanstack.com/guides/important-defaults

});

ReactDOM.render(
(
<ThemeProvider theme={theme}>
<Home />
</ThemeProvider>
<QueryClientProvider client={queryClient}>
<ThemeProvider theme={theme}>
<Home />
</ThemeProvider>
</QueryClientProvider>
),
document.getElementById('react-app')
);
Loading