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

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

Closed

Conversation

ShaunSHamilton
Copy link
Member

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the main branch of freeCodeCamp.
  • I have tested these changes either locally on my machine, or GitPod.

Closes #37615

22/07/2021 This includes the change in solution-form to fail a submission - for testing.

This does not make use of the updateFetchState state, as I am leaving this open for some discussion as how best to use it. ?

This was largely possible thanks to the help of these developer: Oliver

@gitpod-io
Copy link

gitpod-io bot commented Jul 22, 2021

@github-actions github-actions bot added the platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. label Jul 22, 2021
client/src/redux/failed-updates-epic.js Show resolved Hide resolved
client/src/redux/index.js Show resolved Hide resolved
Comment on lines -100 to +114
concat(of(updateSolutionFormValues({})))
switchMap(project => {
const { type, payload } = project;
if (type === challengeActionTypes.updateFailed) {
return of(updateFailed(payload));
}
return of(updateSolutionFormValues({}));
})
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.

@ojeytonwilliams
Copy link
Contributor

Sorry for thinking about this so late, but do we definitely want to wait for a server response before the user can continue? The reason I ask is that this is not how we do things for the rest of the challenges - we just let them continue and save the submission in local storage.

Do we maybe want to keep the existing behaviour, but make sure that the warning persists?

@ShaunSHamilton
Copy link
Member Author

Do we maybe want to keep the existing behaviour, but make sure that the warning persists?

The way I see it, whilst we might not want to unnecessarily delay a Camper from completing challenges quickly sans saving, project submissions are different, and not meant to be "rushed" through. That is, I suspect most Campers want utter-and-complete confirmation their project submission has gone through, and, if not, then they want to try again, or, realise they have submitted something the server is never going to be happy with.

@ojeytonwilliams
Copy link
Contributor

That makes sense. As long as we only change the flow for project submission, that sounds like a plan.

@raisedadead
Copy link
Member

Cleaning things up. Added this to your Trello, so that you can come back and resurrect this when you are ready.

@ShaunSHamilton ShaunSHamilton deleted the fix/backend-submit branch May 12, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show notifications for failed backend project submission
3 participants