core-34 Add view-only option on permissioned relationships #12415

aydun · 2018-07-04T12:02:44Z

(This is a follow-on from #11922 but I can't reopen that one, which itself was a replacement for #11892 )

Overview

This adds a new option to permissioned relationships to provide view-only access. Previously, a permissioned relationship always provides edit access.

Before

A permissioned relationship provides view and update access to the other contact. There is no option for view-only access.

After

The permissioned relationship can provide View and Update or View Only access.

Technical Details

The is_permission_a_b and is_permission_b_a fields are changed from 0/1 boolean to 0/1/2. As previously 0 = no access, 1 = edit (view & update). In addition 2 = view only.

For clarity, the constants CRM_Contact_BAO_Relationship::NONE, CRM_Contact_BAO_Relationship::EDIT, CRM_Contact_BAO_Relationship::VIEW are used.

This means all existing permissioned relationships continue to function as now and provide edit access.

Comments

Unit tests are included for normal and second-degree relationships.
Incorporates @agh1's suggestion to use icons on the relationship screen

https://lab.civicrm.org/dev/core/issues/34

Change is_permission_a_b and is_permission_b_a to 3 values instead of 2. Update schema, DAO, BAO, view/edit/add, search, report

…was mishandled We need to check for: first_degree_relationship.contact_id_a = second_degree_relationship.contact_id_a first_degree_relationship.contact_id_a = second_degree_relationship.contact_id_b first_degree_relationship.contact_id_b = second_degree_relationship.contact_id_a first_degree_relationship.contact_id_b = second_degree_relationship.contact_id_b but the last one was mishandled.

…_a bug

civibot · 2018-07-04T12:02:45Z

(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).

eileenmcnaughton · 2018-07-04T22:30:07Z

@agh1 are you able to re-review this. It looks generally good, sensible & narrowly-scoped to me. I note the upgrade script line is missing but there is a case to be made for getting the rest of the review first as that is the most likely to go stale

@petednz - you may also have an interest

agh1 · 2018-07-06T14:04:41Z

Yes, I'd be happy to review. Can't happen today, but early next week. I'm excited this is happening!

eileenmcnaughton · 2018-07-06T22:19:43Z

@aydun test fail seems unrelated but if you squash your commits & re-push they will run again & hopefully a pretty green tick will follow

aydun · 2018-07-10T10:00:56Z

@eileenmcnaughton - that worked ... pretty green tick now showing!

eileenmcnaughton · 2018-07-11T00:46:43Z

Passing yay. In your court @agh1

agh1 · 2018-07-16T23:52:08Z

Explain (r-explain)
- PASS : The goal/problem/solution have been adequately explained in the PR.
Test results (r-test)
- PASS: The test results are all-clear.
Code quality (r-code)
- PASS: The functionality, purpose, and style of the code seems clear+sensible.
Documentation (r-doc)
- ISSUE: The user documentation should be updated.
- COMMENTS: This shouldn't hold up merging this PR. There's just a passing mention of relationship permissions on the Relationships page, but this will make the screenshot out of date, and the new options should be briefly documented.
Maintainability (r-maint)
- PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
Run it (r-run)
- PASS: Created user "Test 1", granted "Author" role (has no special CiviCRM permissions). Added a relationship with view+edit permission for Test 1 with another contact "Test 2" and one with view-only permission with another contact "Test 3". Logging in as Test 1, I could use a profile in Edit mode to edit Test 2. I could use a profile in View mode to see Test 3, but Edit mode redirected me to the site's home page. Attempting to see any other contact in View mode displayed the error You do not have permission to edit this contact record. Contact the site administrator if you need assistance.
User impact (r-user)
- PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
- COMMENTS: Sight-impaired users will have trouble identifying which relationships have which permissions (this is not a newly-introduced problem, but one that could be resolved relatively easily). I'd recommend that the icons get title attributes saying "X can be viewed by Y" for each instance.
Technical impact (r-tech)
- PASS: The change potentially affects compatibility, but the risks have been sufficiently managed.
- COMMENTS: This is handled as reasonably as possible. The value 1, which represented the combined view and edit permission in the past, continues to mean "view and edit". Consequently, no schema upgrade is necessary. Only one potential issue I could imagine: someone might look for is_permission_a_b to be positive/truthy and act like that's sufficient for assuming the contact has edit permission when in fact the value is 2 and it's just read-only. There's no way to accommodate that situation, and it's the smoothest potential way to upgrade I can think of.

eileenmcnaughton · 2018-07-17T00:20:43Z

merging based on @agh1 thorough review @aydun can you follow up on docs?

agh1 · 2018-07-17T00:34:17Z

I just wrote a follow-up to this that adds the title attribute I mentioned earlier--see #12487.

colemanw · 2018-07-17T14:38:30Z

That eyeball is really creepy looking. Just sayin...

aydun · 2018-07-17T23:31:44Z

@agh1 - many thanks for reviewing, and the title enhancement
@colemanw - any better suggestions?
@eileenmcnaughton - thanks for merging

colemanw · 2018-07-18T16:41:26Z

@aydun I'm not sure if there's anything better, at least it's not as bad as this icon that used to be in older versions of CiviCRM!

But I do think it would be more intuitive to use 2 icons to represent 2 permissions rather than lumping them into 1. So I would do an eye for "view" and an eye plus a pencil for "view + edit"

agh1 · 2018-07-18T17:38:44Z

@colemanw I think a pencil has usually indicated "go edit this" or "this is editable" rather than "this contact has the power to edit". I had suggested in #11922 (comment) fa-pencil-square-o might be distinct enough, along with some other options.

If you're creeped out by fa-eye, there's fa-binoculars, which may be more implictly creepy but less viscerally weird.

I got experimenting: one idea is to put them in squares and give a little color-coding:

Or with binoculars:

The one consideration with separate icons is that you'll never have editing without viewing, and the second icon will just be taking up space.

colemanw · 2018-07-19T15:49:01Z

I understand that the two permissions are intertwined, nevertheless I think having 2 icons to represent 2 permissions is much more clear visually.
Andrew I really like the way you did those icons. I vote for the eye and the pencil. It looks less creepy in a green box ;)

eileenmcnaughton · 2018-07-19T20:21:16Z

It's a less clockwork orange look

aydun · 2018-07-20T10:33:04Z

Yes, I like those - eye and pencil in boxes.

yashodha · 2018-09-07T09:51:20Z

@aydun does this also handle permission on user dashboard? I mean which links to show or not show depending on the permission granted.

aydun added 4 commits July 4, 2018 12:34

Add view-only option on permissioned relationships:

f871c3a

Change is_permission_a_b and is_permission_b_a to 3 values instead of 2. Update schema, DAO, BAO, view/edit/add, search, report

Use icons for permission status

66229fd

Tests for view permission by relationship, including 2nd degree a_b,b…

aade61c

…_a bug

civicrm-builder added the master label Jul 4, 2018

aydun mentioned this pull request Jul 4, 2018

core-34 (WIP) Add view-only option on permissioned relationships: #11922

Closed

eileenmcnaughton added the has-test label Jul 6, 2018

aydun mentioned this pull request Jul 6, 2018

Update to support view_only permission on relationships eileenmcnaughton/nz.co.fuzion.relatedpermissions#16

Merged

Make permission condition more explicit

da2c767

aydun force-pushed the core-34-v2 branch from d73cb3f to da2c767 Compare July 9, 2018 18:24

eileenmcnaughton merged commit d076584 into civicrm:master Jul 17, 2018

eileenmcnaughton added the sig:user interfacing improvement label Jul 17, 2018

agh1 mentioned this pull request Jul 17, 2018

dev/core#34 Add permission details in title attribute of icons #12487

Merged

MegaphoneJon mentioned this pull request Jul 17, 2018

Document "view only" permissioned relationships civicrm/civicrm-user-guide#344

Open

agh1 mentioned this pull request Jul 20, 2018

Icons in colored boxes for relationship permissions #12526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core-34 Add view-only option on permissioned relationships #12415

core-34 Add view-only option on permissioned relationships #12415

aydun commented Jul 4, 2018 •

edited by eileenmcnaughton

civibot bot commented Jul 4, 2018

eileenmcnaughton commented Jul 4, 2018

agh1 commented Jul 6, 2018

eileenmcnaughton commented Jul 6, 2018

aydun commented Jul 10, 2018

eileenmcnaughton commented Jul 11, 2018

agh1 commented Jul 16, 2018

eileenmcnaughton commented Jul 17, 2018

agh1 commented Jul 17, 2018

colemanw commented Jul 17, 2018

aydun commented Jul 17, 2018

colemanw commented Jul 18, 2018

agh1 commented Jul 18, 2018 •

edited

colemanw commented Jul 19, 2018

eileenmcnaughton commented Jul 19, 2018

aydun commented Jul 20, 2018

yashodha commented Sep 7, 2018

core-34 Add view-only option on permissioned relationships #12415

core-34 Add view-only option on permissioned relationships #12415

Conversation

aydun commented Jul 4, 2018 • edited by eileenmcnaughton

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 4, 2018

eileenmcnaughton commented Jul 4, 2018

agh1 commented Jul 6, 2018

eileenmcnaughton commented Jul 6, 2018

aydun commented Jul 10, 2018

eileenmcnaughton commented Jul 11, 2018

agh1 commented Jul 16, 2018

eileenmcnaughton commented Jul 17, 2018

agh1 commented Jul 17, 2018

colemanw commented Jul 17, 2018

aydun commented Jul 17, 2018

colemanw commented Jul 18, 2018

agh1 commented Jul 18, 2018 • edited

colemanw commented Jul 19, 2018

eileenmcnaughton commented Jul 19, 2018

aydun commented Jul 20, 2018

yashodha commented Sep 7, 2018

aydun commented Jul 4, 2018 •

edited by eileenmcnaughton

agh1 commented Jul 18, 2018 •

edited