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-21052 - Activity API - Allow opt-out of CiviCase activity history #10842

Merged
merged 6 commits into from
Aug 17, 2017

Conversation

totten
Copy link
Member

@totten totten commented Aug 9, 2017

Overview

In case-management, it's often useful to track the detailed revision for
each activity.

This patch helps the CiviCase 5 extension change the mechanism used --
instead of adding special records in civicrm_activity with original_id
and is_current_revision, it will rely on the general-purpose revision
history (the shadow tables in log_civicrm_*).

Note: This makes no change to the default behavior of any CiviCase screens.
It is purely an opt-in for sites using CiviCase 5.

Before

If you create a Case and update an Activity in the case using Activity
API, it records an extra record in the civicrm_activity table.

After

You can toggle an option in "Administer => CiviCase => CiviCase Settings"
called "Enable Embedded Activity Revisions".

  • If TRUE, then activity updates lead to new records (as before).
  • If FALSE, then activity updates save directly without any extra rows.

screen shot 2017-08-09 at 4 49 51 pm

(Updates per feedback) The default value for this setting is based on the following rules:

  • If installing a new site, default to FALSE. This ensures a smoother upgrade path to CiviCase 5.
  • If upgrading an existing site, default to TRUE or FALSE depending on whether the table civicrm_activity contains any revision history:
    • If there are records with is_current_revision=0 or original_id IS NOT NULL, then continue to generate more such records.
    • If there are no such records, then don't start generating them anew.

@totten totten changed the title CRM-20152 - Activity API - Allow opt-out of CiviCase activity history CRM-21052 - Activity API - Allow opt-out of CiviCase activity history Aug 9, 2017
'name' => 'civicaseActivityRevisions',
'type' => 'Boolean',
'quick_form_type' => 'YesNo',
'default' => TRUE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default should be false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trade-off:

  • For existing CiviCase users, TRUE maintains the status-quo. It's arguably buggy, but it's the bug they know -- and sometimes folks have implicit dependencies on a long-standing quirks.
  • For new CiviCase adopters, FALSE will leave their data ready to go to CiviCase 5 without any extra migration steps. However, if we're going to change the default, then we really need a more active round of communications.

Copy link
Member

@colemanw colemanw Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I was thinking mostly about new installs, I think it should be FALSE for them. TRUE is ok for existing installs, agree that's safer.

@totten
Copy link
Member Author

totten commented Aug 16, 2017

@colemanw ok, I've made a few changes based on the feedback:

  • The default is more dynamic, and this is now documented in the PR description.
  • Tested all three scenarios manually. (For upgrade-testing, I made new DB snapshots on dmaster build of v4.7.22 -- one with activity revisions, one without -- then, for each snapshot, I ran civibuild ut <mybuild> <snapshotfile> and checked the resulting setting in the UI.)

@colemanw
Copy link
Member

@totten check test failures

@totten
Copy link
Member Author

totten commented Aug 17, 2017

@colemanw the test issue should be addressed now.

@colemanw colemanw merged commit 34caf21 into civicrm:master Aug 17, 2017
@totten totten deleted the master-case-rev branch August 17, 2017 17:13
@totten totten restored the master-case-rev branch August 18, 2017 04:50
@totten totten deleted the master-case-rev branch August 24, 2017 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants