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

[BD-05] [BB-2963] Improve Instructor dashboard #2

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion src/components/value-or-loading/ValueOrLoading.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ function LoadingOrValue({ value }) {
}

LoadingOrValue.propTypes = {
value: PropType.element,
value: PropType.oneOfType([PropType.element, PropType.number]),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having it be a node means it can be anything that can be rendered, so it will match, numbers, strings and other react elements.

Suggested change
value: PropType.oneOfType([PropType.element, PropType.number]),
value: PropType.node,

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

};

LoadingOrValue.defaultProps = {
Expand Down
12 changes: 12 additions & 0 deletions src/data/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,15 @@ export const oraDataShape = PropTypes.objectOf(PropTypes.shape({
done: PropTypes.number,
status: PropTypes.string,
}));

export const oraSummaryDataShape = PropTypes.objectOf(PropTypes.shape({
units: PropTypes.number,
assessments: PropTypes.number,
total: PropTypes.number,
training: PropTypes.number,
peer: PropTypes.number,
self: PropTypes.number,
waiting: PropTypes.number,
staff: PropTypes.number,
done: PropTypes.number,
}));
26 changes: 17 additions & 9 deletions src/ora-dashboard/OraDashboard.jsx
Original file line number Diff line number Diff line change
@@ -1,26 +1,34 @@
import React from 'react';
import { useSelector } from 'react-redux';
import { Spinner } from '@edx/paragon';
import SummaryTable from './summary-table/SummaryTable';
import DataTable from './data-table/DataTable';
import { oraDataShape } from '../data/constants';
import { oraDataShape, oraSummaryDataShape, RequestStatus } from '../data/constants';
import { loadingStatus } from './data/selectors';

function OraDashboard({ data, summary }) {
const status = useSelector(loadingStatus);

function OraDashboard({ data }) {
return (
<main>
<div className="container-fluid">
<h1>Open Responses</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be translatable.

Copy link
Author

Choose a reason for hiding this comment

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

Now it's translatable.

<SummaryTable />
<DataTable data={data} />
{status === RequestStatus.IN_PROGRESS && <Spinner animation="border" variant="primary" />}
{status === RequestStatus.SUCCESSFUL && (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

If this div isn't adding anything, you can remove it and just <> instead.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for noticing that, yes a fragment would be better.

<SummaryTable data={summary} />
<DataTable data={data} />
</div>
)}

</div>
</main>
);
}

OraDashboard.propTypes = {
data: oraDataShape,
};

OraDashboard.defaultProps = {
data: {},
data: oraDataShape.isRequired,
summary: oraSummaryDataShape.isRequired,
};

export default OraDashboard;
6 changes: 4 additions & 2 deletions src/ora-dashboard/OraDashboardContainer.jsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
import React, { useEffect } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { useParams } from 'react-router';
import { selectOraBlocks } from './data/selectors';
import { selectOraBlocks, selectSummary } from './data/selectors';
import { fetchOraBlocks } from './data/thunks';
import OraDashboard from './OraDashboard';

export default function OraDashboardContainer() {
const { courseId } = useParams();
const dispatch = useDispatch();
const oraBlocks = useSelector(selectOraBlocks);
const summary = useSelector(selectSummary);

useEffect(() => {
// The courseId from the URL is the course we WANT to load.
dispatch(fetchOraBlocks(courseId));
}, [courseId]);

return (
<OraDashboard data={oraBlocks} />
<OraDashboard data={oraBlocks} summary={summary} />
);
}
139 changes: 89 additions & 50 deletions src/ora-dashboard/data-table/DataTable.jsx
Original file line number Diff line number Diff line change
@@ -1,67 +1,106 @@
import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import React, { useEffect } from 'react';
import { Table } from '@edx/paragon';
import { useDispatch } from 'react-redux';
import { useParams } from 'react-router';
import LoadingOrValue from '../../components/value-or-loading/ValueOrLoading';
import messages from './messages';
import { fetchOraReports } from '../data/thunks';
import { oraDataShape } from '../../data/constants';

/**
* Sort implementation for Paragon Table
* @param {any} firstElement
* @param {any} secondElement
* @param {string} key
* @param {string} direction
*/
const sort = function sort(firstElement, secondElement, key, direction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think naming the function twice is a bit redundant.

Suggested change
const sort = function sort(firstElement, secondElement, key, direction) {
function sort(firstElement, secondElement, key, direction) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this seems like a utility function, so can probably moved to a common places and imported as such.

const directionIsAsc = direction === 'asc';

if (firstElement[key] > secondElement[key]) {
return directionIsAsc ? 1 : -1;
} if (firstElement[key] < secondElement[key]) {
return directionIsAsc ? -1 : 1;
}
return 0;
};

function DataTable({ intl, data }) {
const { courseId } = useParams();
const dispatch = useDispatch();
useEffect(() => {
Object.keys(data).map((blockId) => data[blockId].status || dispatch(fetchOraReports(courseId, blockId)));
if (Object.keys(data).length > 0) {
const blockId = Object.keys(data)[0];
if (!data[blockId].status) {
dispatch(fetchOraReports(courseId, blockId));
}
}
}, [courseId, data]);

// create a copy of data for sortable Table
const sortableData = Object.values(data).slice();

// define Table columns
const columns = [
{
label: intl.formatMessage(messages.unit_name),
key: 'vertical',
columnSortable: true,
},
{
label: intl.formatMessage(messages.assessment),
key: 'name',
columnSortable: true,
},
{
label: intl.formatMessage(messages.total_responses),
key: 'total',
columnSortable: true,
},
{
label: intl.formatMessage(messages.training),
key: 'training',
columnSortable: true,
},
{
label: intl.formatMessage(messages.peer),
key: 'peer',
columnSortable: true,
},
{
label: intl.formatMessage(messages.self),
key: 'self',
columnSortable: true,
},
{
label: intl.formatMessage(messages.waiting),
key: 'waiting',
columnSortable: true,
},
{
label: intl.formatMessage(messages.staff),
key: 'staff',
columnSortable: true,
},
{
label: intl.formatMessage(messages.final_grade_received),
key: 'done',
columnSortable: true,
},
];

return (
<table className="table">
<thead>
<tr>
<th>
{intl.formatMessage(messages.unit_name)}
</th>
<th>
{intl.formatMessage(messages.assessment)}
</th>
<th>
{intl.formatMessage(messages.total_responses)}
</th>
<th>
{intl.formatMessage(messages.training)}
</th>
<th>
{intl.formatMessage(messages.peer)}
</th>
<th>
{intl.formatMessage(messages.self)}
</th>
<th>
{intl.formatMessage(messages.waiting)}
</th>
<th>
{intl.formatMessage(messages.staff)}
</th>
<th>
{intl.formatMessage(messages.final_grade_received)}
</th>
</tr>
</thead>
<tbody>
{Object.values(data).map(((block) => (
<tr key={block.id}>
<td>{block.vertical}</td>
<td>{block.name}</td>
<td><LoadingOrValue value={block.total} /></td>
<td><LoadingOrValue value={block.training} /></td>
<td><LoadingOrValue value={block.peer} /></td>
<td><LoadingOrValue value={block.self} /></td>
<td><LoadingOrValue value={block.waiting} /></td>
<td><LoadingOrValue value={block.staff} /></td>
<td><LoadingOrValue value={block.done} /></td>
</tr>
)))}
</tbody>
</table>
<Table
data={sortableData}
columns={columns.map(column => ({
...column,
onSort(direction) {
console.log('Sort in direction ', direction, column);
sortableData.sort((firstElement, secondElement) => sort(firstElement, secondElement, column.key, direction));
},
}))}
tableSortable
/>
);
}
DataTable.propTypes = {
Expand Down
2 changes: 2 additions & 0 deletions src/ora-dashboard/data/selectors.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
/* eslint-disable import/prefer-default-export */
export const selectOraBlocks = state => state.blocks;

export const selectSummary = state => state.summary;

export const loadingStatus = state => state.status;
58 changes: 55 additions & 3 deletions src/ora-dashboard/data/slices.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,61 @@ function oraBlocksFromResponse(response) {
return oraBlocks;
}

/**
* Given a get_ora2_responses API response, set statistics for each ORA Block.
* Also assign default value if there is no statistics for any block.
* @param {array} blockIds - list of blockIdx in current course
* @param {object} payload - get_ora2_responses api response
*/
function oraBlockStatisticsFromPayload(blockIds, payload) {
const blocks = {};
const defaultValues = {
status: RequestStatus.SUCCESSFUL,
total: 0,
training: 0,
peer: 0,
self: 0,
waiting: 0,
staff: 0,
done: 0,
};
blockIds.forEach(blockId => {
blocks[blockId] = { ...defaultValues, ...payload[blockId] };
});
return blocks;
}

/**
* Given current state.oraBlocks object, prepares summary.
* @param {object} data - state.oraBlocks
*/
function prepareStatisticsSummary(data) {
const dataKeys = Object.keys(data);
const dataItemArr = dataKeys.map((key) => data[key]);

const sumByKey = (key) => dataItemArr
.map((item) => (item[key] ? item[key] : 0))
.reduce((result, val) => result + val, 0);

const fields = ['total', 'training', 'peer', 'self', 'waiting', 'staff', 'done'];
const summaryData = {
units: dataKeys.length,
assessments: dataKeys.length,
};

fields.forEach(field => {
summaryData[field] = sumByKey(field);
});

return summaryData;
}

const oraSlice = createSlice({
name: 'ora',
initialState: {
status: RequestStatus.IN_PROGRESS,
blocks: {},
summary: {},
},
reducers: {
fetchOraBlocksRequest: (state) => {
Expand All @@ -57,10 +107,12 @@ const oraSlice = createSlice({
state.blocks[payload.blockId].status = RequestStatus.IN_PROGRESS;
},
fetchOraReportSuccess: (state, { payload }) => {
Object.keys(payload).forEach((blockId) => {
state.blocks[blockId].status = RequestStatus.SUCCESSFUL;
Object.assign(state.blocks[blockId], payload[blockId]);
const statistics = oraBlockStatisticsFromPayload(Object.keys(state.blocks), payload);
Object.keys(state.blocks).forEach(blockId => {
state.blocks[blockId] = { ...state.blocks[blockId], ...statistics[blockId] };
});
// as statistics updated, update summary too.
state.summary = prepareStatisticsSummary(state.blocks);
},
fetchOraReportFailed: (state, { payload }) => {
state.blocks[payload.blockId].status = RequestStatus.FAILED;
Expand Down
Loading