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

dev/core#1664 - Fix errors recorded in log message on Manage Case scr… #16846

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

jitendrapurohit
Copy link
Contributor

…een and case dashboard page

Overview

Various errors recorded in log message on

  • Manage Case screen -> relationship tab.
  • Case Dashboard Page.

Before

See gitlab for steps to replicate the issue.

In addition, visiting the case dashboard page records the below error in drupal logs-

Notice: Undefined index: all in CRM_Case_Page_AJAX::getCases() (line 201 of /Users/jitendra/src/civicrm/CRM/Case/Page/AJAX.php).

After

No error message is recorded.

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/1664

@civibot
Copy link

civibot bot commented Mar 19, 2020

(Standard links)

@civibot civibot bot added the master label Mar 19, 2020
@@ -56,7 +56,7 @@ public static function getCaseGlobalRelationships() {
// get the total row count
CRM_Case_BAO_Case::getGlobalContacts($globalGroupInfo, NULL, FALSE, TRUE, NULL, NULL);
// limit the rows
$relGlobal = CRM_Case_BAO_Case::getGlobalContacts($globalGroupInfo, $params['sortBy'], $showLinks = TRUE, FALSE, $params['offset'], $params['rp']);
$relGlobal = CRM_Case_BAO_Case::getGlobalContacts($globalGroupInfo, CRM_Utils_Array::value('sortBy', $params), $showLinks = TRUE, FALSE, $params['offset'], $params['rp']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR - I've noticed these messages too. FYI @colemanw might revoke your Civi card if he sees this line. Use $params['sortBy'] ?? NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @demeritcowboy. Have updated the PR now.

@@ -182,7 +182,7 @@ public static function getCases() {
$params = CRM_Core_Page_AJAX::defaultSortAndPagerParams();
$params += CRM_Core_Page_AJAX::validateParams($requiredParameters, $optionalParameters);

$allCases = (bool) $params['all'];
$allCases = (bool) CRM_Utils_Array::value('all', $params, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

$allCases = !empty($params['all']);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Also checked the My Cases dashlet.
      • I didn't expect it to fix this, but just noting for other reviewers if you're testing sorting there is still a known separate sorting problem (https://lab.civicrm.org/dev/core/issues/1624)
  • Developer standards
    • [r-tech] PASS
    • [r-code] Issue
      • The Array::value ones won't pass final review. They're so 2019.
    • [r-maint] Undecided
    • [r-test] PASS

@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit - @colemanw is on the warpath against CRM_Utils_Array::value - if you swap those out this is mergeable based on the review

@jitendrapurohit
Copy link
Contributor Author

ah, sorry, wasn't aware of that. Noticed loads of warning on case screen and quickly pushed a PR with the fix. I've updated the changes and removed the usage of Utils_Array now 👍

@seamuslee001
Copy link
Contributor

Merging based on @demeritcowboy 's review

@seamuslee001 seamuslee001 merged commit 6769b88 into civicrm:master Mar 20, 2020
@jitendrapurohit jitendrapurohit deleted the core-1664 branch March 24, 2020 08:36
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.

4 participants