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

Format details for case custom data activity in a human readable format #13365

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Dec 28, 2018

Overview

When a Case "Change Custom Data" activity is created the activity details are virtually unreadable to a human! It is a json encoded array of all previous custom data values with their internal names (eg. custom_55).

This PR changes that so that only the actual changes are shown and the custom field labels are used.

Before

"Unreadable" json array of all custom data values before change:
casecustomdataold

After

  • "Human readable" list of custom field labels (as shown on the actual custom data on the case overview).
  • Old and new value for each shown.
  • Values that did not change are not shown.

casecustomdatanewsome
casecustomdatanewall
casecustomdatanewallmulti

Technical Details

A new function is created to format the custom data for the activity.

Comments

@mattwire mattwire changed the title Format details for case custom data activity in a human readable format WIP Format details for case custom data activity in a human readable format Dec 28, 2018
@mattwire mattwire changed the title WIP Format details for case custom data activity in a human readable format Format details for case custom data activity in a human readable format Dec 29, 2018
@mattwire mattwire force-pushed the caseformatcustomdata branch 2 times, most recently from fe41f72 to 1afd762 Compare December 29, 2018 11:20
@seamuslee001
Copy link
Contributor

@mattwire are you able to add a test of the new API i think that would be best here

@mattwire mattwire force-pushed the caseformatcustomdata branch from 1afd762 to 6437dda Compare October 4, 2019 13:41
@mattwire mattwire added merge ready PR will be merged after a few days if there are no objections and removed needs-test labels Oct 4, 2019
@seamuslee001
Copy link
Contributor

@jusfreeman @petednz would either of you folks be interested in reviewing this change

@jusfreeman
Copy link
Contributor

@seamuslee001 yep I can do a review.

@jusfreeman
Copy link
Contributor

@mattwire @seamuslee001 have tested this on 5.18.3 and saving a case with custom fields returns the following error with this PR applied.

$Fatal Error Details = array(3) { ["message"]=> string(87) "API (CustomValue, getdisplayvalue) does not exist (join the API team and implement it!)" ["code"]=> NULL ["exception"]=> object(CiviCRM_API3_Exception)#883 (8) { ["extraParams":"CiviCRM_API3_Exception":private]=> array(5) { ["error_code"]=> string(9) "not-found" ["entity"]=> string(11) "CustomValue" ["action"]=> string(15) "getdisplayvalue" ["is_error"]=> int(1) ["error_message"]=> string(87) "API (CustomValue, getdisplayvalue) does not exist (join the API team and implement it!)" } ["message":protected]=> string(87) "API (CustomValue, getdisplayvalue) does not exist (join the API team and implement it!)"

@mattwire
Copy link
Contributor Author

@jusfreeman It relies on a "new" API call CustomValue.getdisplayvalue that was merged into 5.20 (#15335). The patch should apply cleanly to 5.18.3 if you're able to apply and test?

@jusfreeman
Copy link
Contributor

jusfreeman commented Oct 16, 2019

@mattwire thanks

The patch should apply cleanly to 5.18.3 if you're able to apply and test?

This patch did apply cleanly to 5.18.3 but does not work, I had checked the related PR #15335 and it appeared that it had been merged with 5.18.3 but I can see in the code now that this is not the case.

Honestly, I find it really confusing to identify which version a PR should be applied too, there's no obvious indicators on the PR itself and I have to dig around Github history or simply download the code and see if it exists.

It would be really helpful for reviewers to know which version a PR should be tested against - this would save review time and effort (avoid frustration), and importantly, avoid incorrect test results.

Testing results on 5.20.alpha1.

Before PR applied, results are JSON output.

chrome_VHgv34ENYp

Before PR applied, results are human-readable output.

chrome_ovjbmeDlpb

No errors in CiviCRM log. Looks OK to me, thanks @mattwire

Agileware Ref: CIVICRM-1335

@seamuslee001
Copy link
Contributor

Merging as per @jusfreeman 's testing

@seamuslee001 seamuslee001 merged commit 82ef31e into civicrm:master Oct 16, 2019
@jusfreeman
Copy link
Contributor

@seamuslee001 going forward, can you add a label to identify which version this PR and others apply too? That would be helpful.

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