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

Remove activitystatus js. Add submitOnce handler for activity create #13342

Merged
merged 1 commit into from
Feb 13, 2019

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Dec 21, 2018

Overview

Related to #13333 which has now been merged.

On some sites activities can be duplicated by clicking submit/save multiple times when adding an activity. It does not seem easy to reproduce on other sites (eg. dmaster demo). However, looking at the code and simulating multiple clicks in the browser it is certainly possible to submit and there is no protection against creating multiple activities. So I suspect that slower servers/slower browsers are more likely to create duplicates.

This PR also removes javascript which is meant to warn the user if a scheduled activity is in the past or a completed one in the future. However, this js doesn't work on either dmaster.demo or any of my test/live sites and has not been modified for 5 years. As this code also adds an onclick handler it makes it much more difficult to add the "submitOnce" handler. As it seems like this code has not worked for a long time I think it is sensible to just remove it and simplify the form - a site could implement similar functionality via jquery in an extension if desired?

Before

Possible to create multiple activities by clicking "save" multiple times.

After

Only possible to click "save" once so activity create is only submitted once.

Technical Details

Relies on #13333 which adds an onclick handler to the submit buttons.

Comments

@civibot
Copy link

civibot bot commented Dec 21, 2018

(Standard links)

@civibot civibot bot added the master label Dec 21, 2018
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@jitendrapurohit
Copy link
Contributor

I was able to create more than 1 activities by hitting multiple times on a save button before the form is actually submitted. So this actually replicated on my local setup hence thought of giving it a review :-).

Tested by applying this + #13333 changes on my local - I get a series of warning after submitting the activity form.

image

@mattwire
Copy link
Contributor Author

@jitendrapurohit Thanks for review - I can't reproduce this (no php warnings)... did you have some other customisation, clearcache? or can you provide steps to reproduce?

@jitendrapurohit
Copy link
Contributor

To replicate -

  • Create an activity from civicrm/activity?reset=1&action=add&context=standalone. The form should be opened on a window and not on the ajax dialog box.
  • Click save - the page redirects to the civicrm dashboard.
  • Open any link from the recent items, eg the view activity link which was created from the above point.
  • This should list the notices on the page. It works fine without this PR applied.

Hope that helps.

@mattwire mattwire force-pushed the activity_submitonce branch 2 times, most recently from f49bb71 to c63f27e Compare December 28, 2018 13:13
@mattwire
Copy link
Contributor Author

@jitendrapurohit Thankyou, that should be fixed now. Can you try again?

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@mattwire
Copy link
Contributor Author

@colemanw Thanks for merging #13333 Are you happy to merge this one too? As it follows on from that one.

@mattwire mattwire force-pushed the activity_submitonce branch from c63f27e to e517eb4 Compare February 11, 2019 17:54
@colemanw
Copy link
Member

In my testing, I was able to create 15 activities at once before applying this patch (I'm a fast clicker).
After the patch, only one :)
Good to merge, thanks Matt.

@eileenmcnaughton eileenmcnaughton merged commit 7b3be04 into civicrm:master Feb 13, 2019
@mattwire mattwire deleted the activity_submitonce branch March 1, 2019 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants