Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[new_profile] Pressing create multiple time creates multiple candidates "silently" #5808

Merged
merged 6 commits into from
Nov 29, 2019

Conversation

maltheism
Copy link
Member

@maltheism maltheism commented Nov 28, 2019

Brief summary of changes

Solves the bug where multiple candidates were made from the frontend form submission.

Note this implementation is similar to how it was done before. Another way would be to create a disabled props for the button component.

Testing instructions (if applicable)

  1. Checkout PR.
  2. Go to New Profile and press the submit button as much as you want.

Links to related tickets (GitHub, Redmine, ...)

@maltheism maltheism added Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) 22.0.0 TESTING labels Nov 28, 2019
@maltheism
Copy link
Member Author

Hi @zaliqarosli, I made the changes to make the code better as suggested by passing disable through props.

@driusan
Copy link
Collaborator

driusan commented Nov 28, 2019

@maltheism can you send this to 21.0-release? It's affecting some production instances and needs to go into a bugfix release before 22.0

@maltheism maltheism changed the base branch from 22.0-release to 21.0-release November 28, 2019 19:48
@maltheism
Copy link
Member Author

maltheism commented Nov 28, 2019

@driusan PR rebased to 21.0-release

@driusan
Copy link
Collaborator

driusan commented Nov 28, 2019

@zaliqarosli re-review?

@@ -89,6 +90,9 @@ class NewProfileIndex extends React.Component {
}
formObject.append('fire_away', 'New Candidate');

// disable form from resubmission.
this.setState({disable: true});
Copy link
Collaborator

@HenriRabalais HenriRabalais Nov 28, 2019

Choose a reason for hiding this comment

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

I would call this something a bit more descriptive like disableSubmit or something to that effect. Perhaps submitDisabled because it is written as more of a state (true/false) whereas disable sounds like an action (i.e. function).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I think if the submission fails, the button should be re-enabled so that the user can attempt another submission.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @HenriRabalais, I added the suggested changes.

@@ -103,6 +107,8 @@ class NewProfileIndex extends React.Component {
});
} else {
resp.json().then((message) => {
// enable form from resubmission.
Copy link
Collaborator

Choose a reason for hiding this comment

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

might as well fix the typo here while i test it manually!

Copy link
Member Author

Choose a reason for hiding this comment

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

@HenriRabalais I fixed it. thanks!

Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

looks good!

jsx/Form.js Show resolved Hide resolved
@maltheism
Copy link
Member Author

@HenriRabalais @zaliqarosli @driusan everything was updated for suggestions & modifications.

@HenriRabalais HenriRabalais added the Passed Manual Tests PR has undergone proper testing by at least one peer label Nov 28, 2019
@driusan driusan merged commit 7461730 into aces:21.0-release Nov 29, 2019
@ridz1208 ridz1208 added this to the 21.0.5 milestone Nov 29, 2019
@maltheism maltheism deleted the new_profile_fix_22.0 branch May 24, 2020 02:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants