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

CRM-21507 Fix error when creating case activities #11360

Merged
merged 2 commits into from
Dec 4, 2017

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Dec 3, 2017

Overview

This PR fixes a recent regression from #11264 as described by @jitendrapurohit where the user is unable to create new case activities.

Before

When adding a new activity to a case, an error message "Activity Separation is a required field" appears and prevents the user from submitting the form. There is no way to get around this error.

After

The user can create case activities without hitting this error message. Also the error still displays as expected when creating non-case activities that have multiple target contacts.

Technical details

I wasn't entirely sure of the best way to implement this fix. I considered several different ways and weighed downsides of each. I'm open to other suggestions and further discussion here.

Comments

Sorry for the delay in getting this fix made! I think it'd be good if we could get this change reviewed/merged before cutting the RC. Otherwise we'll need to change this PR to the RC branch to avoid releasing a regression.


Before: When adding a new activity to a case, an error message
"Activity Separation is a required field" appears and prevents the user
from submitting the form. There is no way to get around this error.

After: The user can create case activities without hitting this error
message. Also the error still displays as expected when creating
non-case activities that have multiple target contacts.

This changes fixes a regression caused by my fix for CRM-21419.
@colemanw
Copy link
Member

colemanw commented Dec 3, 2017

I see the validation is now conditional on $supportsActivitySeparation, which makes sense, but shouldn't adding the radio element to the form also be conditional?

Only display the this field on the form if it's defined in the form
class. Only define it in the form class if the class property says to do
so.
@seancolsen
Copy link
Contributor Author

@colemanw Thanks for the quick look. Yeah, good point. I added another commit which I hope addresses what you had in mind. Want to take another look now?

Note: I don't think the js code needs to be conditional on this too, but I can put the conditional there too if you think that'd be a good idea.

@colemanw colemanw merged commit 9dd187f into civicrm:master Dec 4, 2017
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21507 Fix error when creating case activities
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.

3 participants