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

Make it impossible to Create Duplicate Resource Entries #1166

Closed
wants to merge 0 commits into from

Conversation

laura-barluzzi
Copy link
Contributor

Proposed changes in this pull request

Fixed issue Possible to Create Duplicate Resource Entries #1107

Inside the tag to attach a resource I did the following:

  1. Added onclick="this.form.submit(); this.disabled=true;" so that the button gets disabled after the form has been submitted.

  2. Changed name="submit" into name="submit-name" otherwise the system doesn't recognize submit() as a function.

When should this PR be merged

I double-checked the rest of the code to see if there're places where the button name needs to be updated as well. Although I haven't found any, it may be worth double-checking before merging.

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2017

CLA assistant check
All committers have signed the CLA.

@laura-barluzzi laura-barluzzi changed the title Impossible to Create Duplicate Resource Entries Make it impossible to Create Duplicate Resource Entries Feb 28, 2017
Copy link
Member

@oliverroick oliverroick left a comment

Choose a reason for hiding this comment

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

Hi @laura-barluzzi, thank you for your contribution. I have a change request:

Could you think of a way to turn this approach into a more generic form. I believe that it might be handy to have this functionality in an external script so we can reuse it in other forms as well. The way I would approach it is, instead of using onclick I would register an event handler on the submit of the form and disable the button in the handler function. It shouldn't be too difficult using jQuery.

@laura-barluzzi
Copy link
Contributor Author

@oliverroick thanks for your suggestion. Sure I can do that, I'll work on it in the next days.

@oliverroick
Copy link
Member

@laura-barluzzi One thing I just noticed: You've been committing work directly to the master branch of your repo, and you made the PR from master. That's not a good thing to do, because it can make it difficult to incorporate changes from the "upstream" Cadasta/cadasta-platform repository into your work, which you need to do to keep your PR branch up to date with other work that's going on in the main repo. The way to deal with this is to work on what people call a topic (or feature) branch. When you want to work on a feature, you check out your master branch, then do something like git checkout -b bugfix/#675, which makes a new branch with a mnemonic name, expecially for the work you're going to do, so that we can keep track of what's going on.

@oliverroick oliverroick self-assigned this Mar 2, 2017
@laura-barluzzi
Copy link
Contributor Author

laura-barluzzi commented Mar 2, 2017

@oliverroick thanks for telling me, I'll create a branch and work from that instead. Just to clarify, should I name the branch "bugfix/#1107"?

@laura-barluzzi
Copy link
Contributor Author

@oliverroick I've been working on the solution and I was wondering if it's fine to combine both approaches so that I still use the onclick="onlySubmitOnce(this.form)" and I call the external function onlySubmitOnce(form) with the form passed as argument. Would this be acceptable?

@laura-barluzzi
Copy link
Contributor Author

Follow up pull request (submitted from feature branch): #1191. Please review that pull request instead of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants