dev/core#1050 - Delete repeat activities that are selected for deletion

[kirk-circle]

Overview

Ensures that when the user opts to delete multiple repeat activities, the specified activities are actually deleted.

Repeating activities are created by entering details into the "Repeat Activity" section on the New/Edit Activity form. Then when the user clicks "Delete" on a repeat activity, a pop-up appears asking whether to delete "Only this activity", "This activity onwards" or "Every activity".

Before

On the Activities tab, if the user clicks "Delete" on a repeat activity and then opts to delete either "This activity onwards" or "Every activity", the only activity that's deleted is the one whose "Delete" link was clicked.

After

On the Activities tab, if the user clicks "Delete" on a repeat activity and then opts to delete either "This activity onwards" or "Every activity", the specified repeat activities are deleted as well as the one whose "Delete" link was clicked.

Technical Details

Although the pop-up asks the user which repeat activities to delete, it looks like this feature was never actually implemented.

This PR implements the feature as follows: Instead of CRM_Activity_Form_Activity::postProcess deleting a single activity, it builds a list of activities to be deleted and then iterates over that list, deleting each one.

To build a list containing the IDs of the repeat activities to be deleted, the CRM_Core_BAO_RecurringEntity::getEntitiesFor function is called. If the activity is not a repeating activity, then that function will return an empty list, in which case the list will then be set to contain only the ID of the single activity whose "Delete" link was clicked.

[civicrm-builder]

Can one of the admins verify this patch?

[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]

add to whitelist

[kirk-circle]

As requested by @mattwire, I have added a test: CRM_Activity_Form_ActivityTest::testActivityDelete. It creates a number of non-repeating and repeating activities, and then calls the CRM_Activity_Form_Activity::postProcess function (the one that I've patched) to test the following:

• Deleting a non-repeating activity
• Deleting a repeating activity in mode 1 (the specified activity only)
• Deleting a repeating activity in mode 2 (from the specified activity to the end of the series)
• Deleting a repeating activity in mode 3 (the whole series)

After each deletion it checks that the expected activity/activities were deleted and no others.

Is it ready to merge now?

[kirk-circle]

Hmm, not sure why tests failed after I pushed commit c774305. That commit only contains a very minor code change that shouldn't have had any effect on any tests. Perhaps some other change in master caused the failure? Is it possible to rerun the tests?

[demeritcowboy]

@kirk-circle Overall seems good to me. And at some point can you rebase the commits (https://docs.civicrm.org/dev/en/latest/tools/git/#rebasing-interactive). If you get stuck I've found that this article has a nice procedure that has prevented at least one laptop from being smashed with a hammer: https://stackoverflow.com/a/6103022/8332458

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • Edge case, and I don't think this PR needs to change just noting it. And that's what you all get for asking me to review because I'm going to do stuff like this ... he he.
      1. Enable civicase, create a case.
      2. Make a non-case repeat activity.
      3. For one of them, do a file-on-case.
      4. Now choose to delete one of the regular activities and choose every activity.
      5. It deletes them all including the one that was filed on case, and it's not in the trash to restore (as it should normally be when a civicase activity is deleted).
    • Noticed one or two things related to the repeat feature but not related to the changes here - will log as separate tickets.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
    • Line 913 could be replaced with just array_column(), but it's fine as-is too.
  • [r-maint] PASS
    • I also ran it against the current code without the patch and it failed as expected for the last two tests so that's great.
  • [r-test] PASS

[seamuslee001]

Jenkins re test this please

[kirk-circle]

Hi @demeritcowboy, thanks for the review. 🙂

• Line 913 could be replaced with just array_column(), but it's fine as-is too.

Good call. How did I not know about the array_column function?

And at some point can you rebase the commits (https://docs.civicrm.org/dev/en/latest/tools/git/#rebasing-interactive). If you get stuck I've found that this article has a nice procedure that has prevented at least one laptop from being smashed with a hammer: https://stackoverflow.com/a/6103022/8332458

Thanks for the helpful advice. I have squashed the existing commits, together with the above change, into two commits - one for deleting repeat activities, and one for testing the form's deletion of single and repeating activities.

[seamuslee001]

based on @demeritcowboy review i'm going to merge this