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

CPS-427: Civicase alignment with CiviCRM 5.35 #717

Merged
merged 23 commits into from
Mar 25, 2021

Conversation

reneolivo
Copy link
Member

@reneolivo reneolivo commented Mar 1, 2021

Overview

This PR fixes a number of bugs that are currently present on Civicase in order to properly run it in CiviCRM version 5.35.

Note: This PR also includes:

Bug fixes

Note, it's important to understand that apart from core changes, AngularJS has also been upgraded to version 1.8 and this contributes to some of the issues we see.

CiviCRM Dashboard

The CiviCRM Dashboard was breaking because it has been completely redone in AngularJS. Our current implementation that added an activity tab to the dashboard is no longer valid.

We decided to remove the Dashboard for now and implement it at a later ticket since this requires more planning.

Previous Dashboard

pr

Current Dashboard

pr

Technical details

All files related to the CRM Dashboard we implemented have been removed.

Unknown path error

When navigating to any Civicase page we get a "unknown path" message and no actual case management content.

Before

Screen Shot 2021-02-28 at 9 09 47 PM

After

Screen Shot 2021-02-28 at 9 10 55 PM

Technical details

This issue is because the CiviCRM Angular modules could not find the Civicase JS files.

There have been many recent changes to the asset manager:
civicrm/civicrm-core#18247 (as an example, not a specific problem). One of these changes have made it so that full paths to a file are not longer found by core.

ie:

return [
  'js' => [
    'ang/civicase/*.js', // CiviCRM glob works ✅ 
    'ang/relative/path/to/file.js', // works ✅ 
    '/full/path/to/file.js', // doesn't work 🚫 
  ],
];

To fix this issue we updated the GlobRecursive class so it returns files relatively to the extension's path. We updated all relevant angular modules so they use the getRelativeToExtension method.

Manage cases errors

When visiting the manage cases page, we get AngularJS infinite digest loop errors.

Before

Screen Shot 2021-02-28 at 9 39 27 PM

After

Screen Shot 2021-02-28 at 9 10 55 PM

Technical details

The problem happens because the AngularJS Strict Contextual Escaping service's methods now return a new reference each time they are called. We were using getActivityFeedUrl (which uses $sce.trustAsResourceUrl under the hood) directly on templates and since this would return a new reference the digest would not stop.

To stop this we removed the $sce.trustAsResourceUrl from getActivityFeedUrl. We have done a quick smock test and this has not broken any major area. The reason for using $sce.trustAsResourceUrl was also never documented.

Broken Activities tab

The Activities tab is missing the archive list on the right sidebar.

Before

Screen Shot 2021-02-28 at 10 17 13 PM

After

Screen Shot 2021-02-28 at 10 18 33 PM

Technical details

The problem was that the Activity.Getmonthswithactivities endpoint was breaking because the _civicrm_api3_get_BAO(__FUNCTION__) call was not returning the CRM_Activity_BAO_Activity BAO class. This is probably related to the refactoring that was done to split civicrm_api3_activity_Getmonthswithactivities into multiple functions to make them more readable, but now _civicrm_api3_get_BAO(__FUNCTION__) was not able to determine the BAO class we needed using the name of the function. We tried renaming the function but this did not work.

Instead of relying on the function name to determine the BAO file we are 100% sure that we need the CRM_Activity_BAO_Activity BAO class we pass it directly instead of using _civicrm_api3_get_BAO. This makes the function more clear.

Failing unit tests

Some of the unit tests failed after upgrading to the new Angular version.

  • Civicase's repo test actions have been updated to use the new CiviCRM 5.35 version.
  • Mocked $broadcast events needed to have a callThrough appended. Here is a good explanation for the fix: https://stackoverflow.com/questions/22721657/typeerror-cannot-read-property-defaultprevented-of-undefined
  • After we removed the $sce.trustAsResourceUrl method from getActivityFeedUrl we should have removed the test with the expectation. As a note, there was a PR that added this test, but again, no explanation why $sce.trustAsResourceUrl was needed since the solution works without it just fine.
  • Workflow tests were failing because we were trying to add a controller to the /caseType/:id route, but this route was never defined so all workflow tests were failing. We created a mock route and updated the karma configuration file so it include mocks and mock modules before unit tests run.
  • One of the broadcast tests was always succeeding in our previous implementation because no $broadcast events were triggered at all. Now that $broadcast events are triggering it started failing so we defined the specific $broadcast event that we are not expecting.
  • Some workflow tests were failing because of an unhandled promise. We patched this using $qProvider.errorOnUnhandledRejections(false); on the spec file, but we need to confirm if we are properly handling rejections on the implementation.
  • Some PHP unit tests were failing because the CRM_Civicase_APIHelpers_CaseDetails class was calling an internal private method statically from a static function, but the private method was not static. We changed the method so it's static.

@reneolivo reneolivo added the bug Something isn't working label Mar 1, 2021
@reneolivo reneolivo force-pushed the CPS-427-civicase-civicrm-5.35-alignment branch from 8211d2c to bc5bd5c Compare March 3, 2021 02:44
@deb1990
Copy link

deb1990 commented Mar 3, 2021

@reneolivo Great job.
But please

  1. Mention why we removed the Dashboard.
  2. Technical Details for "Broken People's tab dates" is missing.

@reneolivo
Copy link
Member Author

@deb1990 thanks a lot. Regarding your comments:

1 - added why the Dashboard was removed.
2 - Removed the "Broken People's tab dates" section because that fix was related to files that were removed in this PR: #714 since the solution doesn't need the files we updated here we just removed this commit.

deb1990
deb1990 previously approved these changes Mar 3, 2021
@reneolivo reneolivo force-pushed the CPS-427-civicase-civicrm-5.35-alignment branch from 58a3bfd to 50e6a3a Compare March 3, 2021 14:01
Copy link

@lisandro-compucorp lisandro-compucorp left a comment

Choose a reason for hiding this comment

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

👍

…ase-upgrader

COMCL-101: Regenerate civix file and Base upgrader class
@deb1990 deb1990 merged commit af78687 into master Mar 25, 2021
@deb1990 deb1990 deleted the CPS-427-civicase-civicrm-5.35-alignment branch March 25, 2021 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants