dev/core#1499 - Case Resource shows contact names that are not access…

[jitendrapurohit]

…ible to logged in user

Overview

Fix case resource section from loading inaccessible contacts.

Before

Replicate the problem by following the below steps -

1. Add contact A, B and C to case resource group.
2. Create a case for User1 and make sure that user1 is able to access only contact A (add appropriate ACL or relationships, etc).
3. Log in as User1 and navigate to Manage case screen -> open "Other relationship" panel.
4. Notice that "Case Resources" section displays all the 3 contacts A, B and C in the list instead of showing only A to the user.

After

as admin, I can see all contacts.

Masquerading as User1, I can see only accessible contacts from case resources -

Comments

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

Added unit test.

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

This seems fine to me @demeritcowboy @alifrumin thoughts?

[demeritcowboy]

Looks good just one minor thing in the test which needs updating. And I added a historical note in the lab ticket for the record.

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • Reproduced via ACLs and patch prevents it showing on the Manage Case screen.
    • Test fails before patch and passes after.
• Developer standards
  • [r-tech] PASS
    • Also did a quick look around to see if it was used elsewhere. This also hides it from the Send Copy section on case activities, and that seems consistent.
  • [r-code] PASS
  • [r-maint] Minor Issue
    • Technically the assertEquals() are all backwards. The expected value should be the first parameter, otherwise when it fails the message "Failed asserting that 0 matches expected 1" can be confusing.
    • Not a blocker. It seems like the test is checking against CMS permissions, and I couldn't see how to reproduce just using CMS permissions, so the test might not prevent regress, but it's still a good test that adds coverage to a function not previously covered.
  • [r-test] PASS

[yashodha]

@jitendrapurohit looks good. Can you look at @demeritcowboy 's suggestion, the patch seems good to merge though.

[seamuslee001]

I'm going to merge this and do the follow up PR as necessary