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

dev/core#2825 - Make source contact required for activities on the form #22243

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Dec 13, 2021

Overview

https://lab.civicrm.org/dev/core/-/issues/2825

Before

It's possible to edit a case activity and remove the source contact. You don't notice it on the regular activity form because postprocess will fill in the current user, but on a case activity it stays blank and then causes problems later such as disappearing from manage case and crashing the Print Report.

After

Both types of activities have it required.

Technical Details

It's a quickfix because you'll still have case problems if you permanently delete a contact (https://lab.civicrm.org/dev/core/-/issues/1251). That could be considered a bigger issue though because you've lost important data if you do that.

Comments

@civibot
Copy link

civibot bot commented Dec 13, 2021

(Standard links)

@civibot civibot bot added the master label Dec 13, 2021
@colemanw
Copy link
Member

Makes sense... except that if postprocess fills it with current user then is it a normal workflow to leave it blank?

@demeritcowboy
Copy link
Contributor Author

The default is always the logged in user, so it's never blank to start.

@demeritcowboy
Copy link
Contributor Author

The other functions call parent::XX so share with the regular activity. The case activity doesn't call parent for post process. So a different quickfix would be to copy the code from postprocess that fills in the current user if blank.

@colemanw
Copy link
Member

In that case I think this fix is fine.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Dec 13, 2021
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@demeritcowboy
Copy link
Contributor Author

Hmm, any updates made to jenkins itself? This seems to happen after all the tests have run:

ERROR: Step ‘Publish xUnit test result report’ failed: The result file '/home/jenkins/workspace/CiviCRM-Core-PR/junit/CRM_AllTests.xml' for the metric 'JUnit' is not valid. The result file has been skipped.

@seamuslee001
Copy link
Contributor

yeh I upgraded the xunit plugin and its doing interesting things

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@kainuk
Copy link
Contributor

kainuk commented Dec 14, 2021

I just checked it on the generated test. It works fine.
Selection_001

@colemanw colemanw merged commit 22137bd into civicrm:master Dec 14, 2021
@demeritcowboy
Copy link
Contributor Author

Thanks!

@demeritcowboy demeritcowboy deleted the quickfix-reportedby branch December 15, 2021 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants