Migrate custom file attachments to new case on reassignment.

[agilewarealok]

Overview

Steps to reproduce the bug:

1. Create a Custom Data Group for Cases and a "File" Field within it
2. Create a new Case, including an uploaded file in the created field
3. Verify the uploaded file can be downloaded
4. "Reassign" the Case to another Contact
5. Attempt to download the updated file

That does not work, CiviCRM Redirects to Contact's Cases Tab with bounce message "Could not retrieve File"

Before

Custom file attachments are not migrated to the new case.

After

Custom file attachments are migrated to the new case and downloadable.

Comments

Agileware Ref: CIVICRM-967

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

This looks good to me @colemanw @mattwire thoughts?

[eileenmcnaughton]

I like the test!

We are adding new functionality specifically to the case entity so we should think about whether there are any cases for this for other entities or it truly is case-specific. Also, we are by-passing the api so we are bypassing hooks and acls (the latter might not be relevant) when we use the dao methods

[eileenmcnaughton]

Also - how does this relate to the custom field handling we added to the DAO class to copy custom files?

[mattwire]

A big part of the mergeCases() function is basically a "copy case entity" function. We should definitely not be adding more duplicated code here to copy custom files as we already have the CRM_Core_BAO_File::copyEntityFile() function but the better fix would be to replace the whole block of code that copies custom fields with CRM_Core_DAO::copyCustomFields().
As there is a test included with this PR it should be quite easy to test if that works.

[mattwire]

Test failure is related.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@agilewarealok it's a style failure

[agilewarealok]

I think DAO is already handling the custom fields copy in
copyGeneric function but not sure if it is working well for the files.

I've removed my copy files code and now the Test case I've written is throwing an error.

Can someone guide on this? So we can get this merged in. @eileenmcnaughton @mattwire

[eileenmcnaughton]

@agilewarealok I took a look - the issue is the file doesn't exist so the call to CRM_Core_BAO_File::path doesn't retrieve it

I took a look at how api_v3_AttachmentTest works & it has a fn like this

  protected function setUp() {
    parent::setUp();
    $this->useTransaction(TRUE);

    $this->cleanupFiles();
    file_put_contents($this->tmpFile('mytest.txt'), 'This comes from a file');
  }

What we've been doing is moving utilities in the test code base that support specific types of testing to traits - e.g CRMTraits_ACL_PermissionTrait

I think maybe we want a

CRMTraits_Core_FileTrait

& then move the functions from AttachmentTest that are needed here into that trait & use it from this test (otherwise it's duplication)

[demeritcowboy]

Separate from the test, the code in copyGeneric is duplicating the code at https://github.com/civicrm/civicrm-core/blob/master/CRM/Case/BAO/Case.php#L2110, so I get a duplicate INSERT error when I try to reassign a case with a custom field (of any type not just file). If the plan is to use copyGeneric/copyCustomFields then need to also remove the duplicate code?

[jusfreeman]

@eileenmcnaughton please close this PR. I think we'll re-submit a new one.

[eileenmcnaughton]

@jusfreeman - sure - but can't you close them?

[jusfreeman]

Unit test has been copied to #15051