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.

Sign up for GitHub

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

Jump to bottom

fix(client): no submit on server fail #42973

Closed
wants to merge 2 commits into from
Closed

fix(client): no submit on server fail #42973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5f100d
fix(client): no submit on server fail
ShaunSHamilton Jul 20, 2021
aeec764
working with submit break
ShaunSHamilton Jul 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion client/src/redux/failed-updates-epic.js
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@ import {
import postUpdate$ from '../templates/Challenges/utils/postUpdate$';
import { isGoodXHRStatus } from '../templates/Challenges/utils';
import { backEndProject } from '../../utils/challengeTypes';
import { createFlashMessage } from '../components/Flash/redux';

const key = 'fcc-failed-updates';

Expand All @@ -41,7 +42,15 @@ function failedUpdateEpic(action$, state$) {
store.set(key, [...failures, payload]);
}
}),
map(() => onlineStatusChange(false))
map(({ payload }) => {
if (payload?.type === 'error') {
ojeytonwilliams marked this conversation as resolved.
Show resolved Hide resolved
return createFlashMessage({
type: 'danger',
message: payload.message
});
}
return onlineStatusChange(false);
})
);

const flushUpdates = action$.pipe(
Expand Down
11 changes: 11 additions & 0 deletions client/src/redux/index.js
View file
Open in desktop
Expand Up @@ -578,6 +578,17 @@ export const reducer = handleActions(
}
};
},
[types.updateFailed]: (state, { payload }) => {
return {
...state,
updateFetchState: {
pending: false,
complete: false,
ojeytonwilliams marked this conversation as resolved.
Show resolved Hide resolved
errored: true,
error: payload
}
};
},
[challengeTypes.challengeMounted]: (state, { payload }) => ({
...state,
currentChallengeId: payload
Expand Down
2 changes: 1 addition & 1 deletion client/src/templates/Challenges/projects/backend/Show.tsx
View file
Open in desktop
Expand Up @@ -237,7 +237,7 @@ class BackEnd extends Component<BackEndProps> {
challengeType={challengeType}
// eslint-disable-next-line @typescript-eslint/unbound-method
onSubmit={this.handleSubmit}
updateSolutionForm={updateSolutionFormValues}
updateSolutionFormValues={updateSolutionFormValues}
/>
<ProjectToolPanel
guideUrl={getGuideUrl({ forumTopicId, title })}
Expand Down
2 changes: 1 addition & 1 deletion client/src/templates/Challenges/projects/frontend/Show.tsx
View file
Open in desktop
Expand Up @@ -185,7 +185,7 @@ class Project extends Component<ProjectProps> {
description={description}
// eslint-disable-next-line @typescript-eslint/unbound-method
onSubmit={this.handleSubmit}
updateSolutionForm={updateSolutionFormValues}
updateSolutionFormValues={updateSolutionFormValues}
/>
<ProjectToolPanel
guideUrl={getGuideUrl({ forumTopicId, title })}
Expand Down
6 changes: 3 additions & 3 deletions client/src/templates/Challenges/projects/solution-form.tsx
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@ interface FormProps extends WithTranslation {
challengeType: number;
description?: string;
onSubmit: (arg0: SubmitProps) => void;
updateSolutionForm: (arg0: Record<string, unknown>) => void;
updateSolutionFormValues: (arg0: Record<string, unknown>) => void;
}

interface ValidatedValues {
Expand All @@ -34,14 +34,14 @@ export class SolutionForm extends Component<FormProps> {
}

componentDidMount(): void {
this.props.updateSolutionForm({});
this.props.updateSolutionFormValues({});
}

handleSubmit = (validatedValues: ValidatedValues): void => {
// Do not execute challenge, if errors
if (validatedValues.errors.length === 0) {
// updates values on store
this.props.updateSolutionForm(validatedValues.values);
this.props.updateSolutionFormValues({}); // validatedValues.values);
if (validatedValues.invalidValues.length === 0) {
this.props.onSubmit({ isShouldCompletionModalOpen: true });
} else {
Expand Down
53 changes: 37 additions & 16 deletions client/src/templates/Challenges/redux/completion-epic.js
View file
Open in desktop
Expand Up @@ -25,7 +25,8 @@ import {
submitComplete,
updateComplete,
updateFailed,
usernameSelector
usernameSelector,
types as challengeActionTypes
} from '../../../redux';

import postUpdate$ from '../utils/postUpdate$';
Expand All @@ -35,17 +36,24 @@ import { getVerifyCanClaimCert } from '../../../utils/ajax';
function postChallenge(update, username) {
const saveChallenge = postUpdate$(update).pipe(
retry(3),
switchMap(({ points }) =>
of(
submitComplete({
username,
points,
...update.payload
}),
updateComplete()
)
),
catchError(() => of(updateFailed(update)))
switchMap(value => {
if (value.type === 'error') {
return of(updateFailed(value));
} else {
return of(
submitComplete({
username,
points: value.points,
...update.payload
}),
updateComplete()
);
}
}),
catchError((err, caught) => {
console.log(err, caught);
return of(updateFailed(update));
})
);
return saveChallenge;
}
Expand Down Expand Up @@ -97,7 +105,13 @@ function submitProject(type, state) {
payload: challengeInfo
};
return postChallenge(update, username).pipe(
concat(of(updateSolutionFormValues({})))
switchMap(project => {
const { type, payload } = project;
if (type === challengeActionTypes.updateFailed) {
return of(updateFailed(payload));
}
return of(updateSolutionFormValues({}));
})
Comment on lines -100 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this. The issue was, I thought, that action with type == submitChallenge was being dispatched twice, one of which would trigger the error flash and one of which would close the modal and navigate.

This solves the problem, but only by conditionally dropping the actions that're piped in. Instead, should we simply only dispatch a single action?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure. I thought it needed to dispatch one of those actions, in order for the chain to continue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just not convinced two submitChallenge actions should be dispatched.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahmadabdolsaheb am I missing something here? What we're trying to achieve is:

User clicks submit -> client talks to server -> server responds with error -> user is warned

or

User clicks submit -> client talks to server -> server responds with success -> modal is closed -> navigate to next page

and what I'm worried about is that the action that's dispatched to trigger this seems to get dispatched twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

After chatting with @ahmadabdolsaheb we realised that ideally it should both ignore duplicate submitChallenges and just dispatch a single action. Reason being rapid submission is almost certainly a bug, so we should code defensively. And, well, if a single click creates two submissions that's definitely a bug.

);
}

Expand Down Expand Up @@ -162,9 +176,16 @@ export default function completionEpic(action$, state$) {
};

return submitter(type, state).pipe(
concat(closeChallengeModal),
filter(Boolean),
finalize(async () => navigate(await pathToNavigateTo()))
switchMap(({ type, payload }) => {
if (payload.type === 'error') {
return of({ type, payload }).pipe(concat(closeChallengeModal));
}
return of({ type, payload }).pipe(
concat(closeChallengeModal),
filter(Boolean),
finalize(async () => navigate(await pathToNavigateTo()))
);
})
);
})
);
Expand Down
1 change: 1 addition & 0 deletions client/src/templates/Challenges/redux/index.js
View file
Open in desktop
Expand Up @@ -57,6 +57,7 @@ export const types = createTypes(
'initLogs',
'updateConsole',
'updateChallengeMeta',
'updateFailed',
'updateFile',
'updateJSEnabled',
'updateSolutionFormValues',
Expand Down