Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# frozen_string_literal: true
# Reaps aged marketplace preview submissions on a TTL, scheduled weekly via `config/schedule.yml`
# (`Course::Assessment::Marketplace::PreviewContainerService`'s container course is the only source of
# these submissions). Never touches the container course, the assessment copies, or the previewers'
# enrolments — those are deliberately persistent and reused across preview sessions.
class Course::Assessment::Marketplace::PreviewSubmissionReapingJob < ApplicationJob
# Keyed on `updated_at` (last activity), not `created_at`, so an in-progress rehearsal is never
# reaped out from under someone still working, and async autograding has time to land before a
# destroy could race it.
#
# This is the floor on how long an attempt is kept, not the ceiling: the weekly cron means an aged
# submission may linger up to a week past it.
PREVIEW_SUBMISSION_TTL = 24.hours

# Cap deletions per run to avoid bricking the worker (mirrors UserEmailDatabaseCleanupJob). Note
# this caps a WEEK's reaping, not an hour's: if preview volume ever exceeds it, aged submissions
# accumulate faster than they are removed and the cron needs raising before this does.
REAP_BATCH_SIZE = 1000

def perform
ActsAsTenant.without_tenant do
reap_aged_preview_submissions
end
end

private

def reap_aged_preview_submissions
User.with_stamper(User.system) do
Course::Assessment::Submission.transaction do
aged_preview_submissions.group_by(&:assessment).each do |assessment, submissions|
creator_ids = []
submissions.each do |submission|
submission.destroy!
creator_ids << submission.creator_id
end

Course::Assessment::Submission::MonitoringService.destroy_all_by(assessment, creator_ids)
end
end
end
end

# Derived from the course, not a deep join: `Course::Assessment` is `acts_as` a
# `Course::LessonPlan::Item`, so a `joins(assessment: { tab: :category })` chain is fragile.
def aged_preview_submissions
preview_assessment_ids = Course.where(preview: true).flat_map { |course| course.assessments.pluck(:id) }

Course::Assessment::Submission.
includes(:assessment).
where(assessment_id: preview_assessment_ids).
where(updated_at: ...PREVIEW_SUBMISSION_TTL.ago).
limit(REAP_BATCH_SIZE)
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,12 @@ json.submission do
json.basePoints assessment.base_exp
json.bonusPoints assessment.time_bonus_exp
json.pointsAwarded submission.current_points_awarded

# Marketplace preview sandbox only: hand back the auto-grading job this very request enqueued
# (Course::Assessment::Submission#auto_grading_job) so the preview page can poll it and show the
# marks in place. Absent outside a preview course, and absent on any request that did not itself
# finalise the submission.
if current_course.preview? && submission.auto_grading_job
json.autoGradingJobUrl job_path(submission.auto_grading_job.job)
end
end
40 changes: 33 additions & 7 deletions client/app/bundles/common/ErrorPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ import {
Attributions,
useSetAttributions,
} from 'lib/components/wrappers/AttributionsProvider';
import { useAppContext } from 'lib/containers/AppContainer';
import { getCourseIdFromString } from 'lib/helpers/url-helpers';
import {
getForbiddenSourceURL,
getNotFoundSourceURL,
getSuspendedSourceURL,
} from 'lib/hooks/router/redirect';
import { useAppDispatch, useAppSelector } from 'lib/hooks/store';
Expand All @@ -39,6 +41,11 @@ const translations = defineMessages({
defaultMessage:
"Check if you've typed the correct address, try again later, or <home>go back home</home>.",
},
notFoundSubtitleWithoutHome: {
id: 'app.ErrorPage.notFoundSubtitleWithoutHome',
defaultMessage:
"Check if you've typed the correct address, or try again later.",
},
notFoundIllustrationAttribution: {
id: 'app.ErrorPage.notFoundIllustrationAttribution',
defaultMessage:
Expand Down Expand Up @@ -135,6 +142,20 @@ const ErrorPage = (props: ErrorPageProps): JSX.Element => {
const NotFoundPage = (): JSX.Element => {
const { t } = useTranslation();

// A marketplace previewer has nowhere to go back to: `/` resolves to the preview container, their
// only course, and the sandbox lock denies it, so the link would land them on a 403. The link is
// dropped rather than repointed, and it takes a second message rather than a conditional chunk.
const { isPreviewRestricted } = useAppContext();

// Most viewers reach this page because no route matched their URL, and the address bar already
// reads what they typed. The rest are redirected here from a route that did match but whose record
// turned out missing, and arrive carrying that address — put it back, so both look the same.
const sourceURL = getNotFoundSourceURL(window.location.href);

useEffectOnce(() => {
if (sourceURL) window.history.replaceState(null, '', sourceURL);
});

return (
<ErrorPage
attributions={[
Expand Down Expand Up @@ -166,13 +187,18 @@ const NotFoundPage = (): JSX.Element => {
]}
illustrationAlt="Not found illustration"
illustrationSrc={notFoundIllustration}
subtitle={t(translations.notFoundSubtitle, {
home: (chunk) => (
<Link to="/" variant="body1">
{chunk}
</Link>
),
})}
subtitle={
isPreviewRestricted
? t(translations.notFoundSubtitleWithoutHome)
: t(translations.notFoundSubtitle, {
home: (chunk) => (
<Link to="/" variant="body1">
{chunk}
</Link>
),
})
}
tip={sourceURL ?? undefined}
title={t(translations.notFound)}
/>
);
Expand Down
82 changes: 82 additions & 0 deletions client/app/bundles/common/__test__/ErrorPage.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { render } from 'test-utils';
import { HomeLayoutData } from 'types/home';

import ErrorPage from '../ErrorPage';

// `NotFoundPage` renders inside `CourselessContainer`'s outlet, which forwards the root payload as the
// outlet context `useAppContext()` reads. There is no outlet here, so the payload is supplied directly.
const mockAppContext: HomeLayoutData = { locale: 'en', timeZone: null };

jest.mock('lib/containers/AppContainer', () => ({
...jest.requireActual('lib/containers/AppContainer'),
useAppContext: (): HomeLayoutData => mockAppContext,
}));

describe('NotFoundPage', () => {
beforeEach(() => {
delete mockAppContext.isPreviewRestricted;
window.history.replaceState(null, '', '/');
});

it('offers a link home', async () => {
const page = render(<ErrorPage.NotFound />);

expect(
(await page.findByText('go back home')).closest('a'),
).toHaveAttribute('href', '/');
});

it('omits the link home for a restricted previewer', async () => {
mockAppContext.isPreviewRestricted = true;

const page = render(<ErrorPage.NotFound />);

expect(
await page.findByText(
"Check if you've typed the correct address, or try again later.",
),
).toBeInTheDocument();
expect(page.queryByText('go back home')).not.toBeInTheDocument();
});

// A 404 raised after a route already matched arrives here by redirect, so the address bar reads
// `/404` rather than the page the viewer actually asked for. Putting it back is what makes this
// read like the catch-all's not-found page, which never leaves the address it was typed at.
describe('when redirected from a page whose record was missing', () => {
const sourceURL = '/courses/8/assessments/33/submissions/818/edit';

beforeEach(() => {
window.history.replaceState(
null,
'',
`/404?from=${encodeURIComponent(sourceURL)}`,
);
});

it('restores the address it was redirected from', async () => {
const page = render(<ErrorPage.NotFound />);

await page.findByText("That location doesn't exist in this universe...");

expect(window.location.pathname + window.location.search).toBe(sourceURL);
});

it('names that address rather than /404', async () => {
const page = render(<ErrorPage.NotFound />);

expect(await page.findByText(sourceURL)).toBeInTheDocument();
expect(page.queryByText('/404')).not.toBeInTheDocument();
});
});

// The catch-all reaches this page without a redirect, so there is nothing to restore and the
// address is already the right one to show.
it('leaves an address it was not redirected to alone', async () => {
window.history.replaceState(null, '', '/courses/8/nonsense');

const page = render(<ErrorPage.NotFound />);

expect(await page.findByText('/courses/8/nonsense')).toBeInTheDocument();
expect(window.location.pathname).toBe('/courses/8/nonsense');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import CourseAPI from 'api/course';
import { redirectToNotFound } from 'lib/hooks/router/redirect';

import { fetchSubmission, loadSubmissionPage } from '../index';

jest.mock('api/course');
jest.mock('lib/hooks/router/redirect', () => ({
redirectToNotFound: jest.fn(),
}));

// Minimal stand-in for the redux-thunk middleware: recursively invokes any
// dispatched thunk (function) and records every plain action object. Mirrors the
// helper in publish.test.js and finalise.test.js.
const runThunk = async (thunk) => {
const dispatched = [];
const dispatch = (action) => {
if (typeof action === 'function') return action(dispatch, () => ({}));
dispatched.push(action);
return action;
};
await thunk(dispatch, () => ({}));
return dispatched;
};

describe('fetchSubmission', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('hands the axios error to onError when the fetch fails', async () => {
const error = { response: { status: 404 } };
CourseAPI.assessment.submissions.edit.mockRejectedValue(error);
const onError = jest.fn();

await runThunk(fetchSubmission(42, undefined, onError));

expect(onError).toHaveBeenCalledWith(error);
});

it('does not call onError when the fetch succeeds', async () => {
CourseAPI.assessment.submissions.edit.mockResolvedValue({
data: {
submission: { id: 42 },
questions: [],
answers: [],
history: { questions: [] },
},
});
const onError = jest.fn();

await runThunk(fetchSubmission(42, undefined, onError));

expect(onError).not.toHaveBeenCalled();
});

it('still dispatches FETCH_SUBMISSION_FAILURE when onError is omitted', async () => {
CourseAPI.assessment.submissions.edit.mockRejectedValue({
response: { status: 500 },
});

const dispatched = await runThunk(fetchSubmission(42));

expect(
dispatched.some((action) => action.type === 'FETCH_SUBMISSION_FAILURE'),
).toBe(true);
});
});

describe('loadSubmissionPage', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('sends the viewer to the not-found page when the fetch 404s', async () => {
CourseAPI.assessment.submissions.edit.mockRejectedValue({
response: { status: 404 },
});

await runThunk(loadSubmissionPage(42));

expect(redirectToNotFound).toHaveBeenCalled();
});

// Only a 404 means "no such submission under this assessment". This matters beyond tidiness: the
// marketplace preview banner reads the very same 404 as a purged sandbox and has its own message
// for it, which is why it refetches through `fetchSubmission` directly rather than through here.
it('stays on the page on any other failure', async () => {
CourseAPI.assessment.submissions.edit.mockRejectedValue({
response: { status: 500 },
});

const dispatched = await runThunk(loadSubmissionPage(42));

expect(redirectToNotFound).not.toHaveBeenCalled();
expect(
dispatched.some((action) => action.type === 'FETCH_SUBMISSION_FAILURE'),
).toBe(true);
});

it('stays on the page when the fetch succeeds', async () => {
CourseAPI.assessment.submissions.edit.mockResolvedValue({
data: {
submission: { id: 42 },
questions: [],
answers: [],
history: { questions: [] },
},
});

await runThunk(loadSubmissionPage(42));

expect(redirectToNotFound).not.toHaveBeenCalled();
});
});
Loading