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

Relationship view/edit/delete quickform - don't require cid in url params #20883

Closed
wants to merge 1 commit into from

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 16, 2021

Overview

This simplifies the URL arguments needed in the relationship view/edit/delete form.

Before

Three URL args required: 'cid', 'id' and 'rtype'.

After

The redundant 'cid' param is looked up based on the other two.

Comments

This makes constructing links from SearchKit much simpler, and also fixes a potential buggy situation where leaving the 'cid' param off the url tricks the form into thinking the relationship is for the current user.

Previously 'cid' was required in addition to 'id' and 'rtype'.
The redundant param can be looked up based on relationship id and r_type (direction).
@civibot
Copy link

civibot bot commented Jul 16, 2021

(Standard links)

@civibot civibot bot added the master label Jul 16, 2021
@colemanw colemanw changed the title Relationship view/edit/delete form - accept no cid in url params Relationship view/edit/delete quickform - accept no cid in url params Jul 17, 2021
@colemanw colemanw changed the title Relationship view/edit/delete quickform - accept no cid in url params Relationship view/edit/delete quickform - don't require cid in url params Jul 17, 2021
@@ -100,7 +100,7 @@ public function view() {
$rType = CRM_Utils_Array::value('rtype', $viewRelationship[$this->getEntityId()]);
// add viewed contribution to recent items list
$url = CRM_Utils_System::url('civicrm/contact/view/rel',
"action=view&reset=1&id={$viewRelationship[$this->getEntityId()]['id']}&cid={$this->getContactId()}&context=home"
"action=view&reset=1&id={$viewRelationship[$this->getEntityId()]['id']}&context=home"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing rtype

Copy link
Contributor

Choose a reason for hiding this comment

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

is rtype needed if we have the relationship id?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally no - I think @demeritcowboy is suggesting it doesn't work right without it with the code as currently is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right - at the moment the way the destination page works, without rtype you get a mostly blank page when you click the recent items view link.

@@ -109,6 +109,7 @@ public function getContactId() {
public function setContactId($contactId) {
$this->_contactID = $contactId;
$this->_contactId = $contactId;
$this->assign('contactId', $this->getContactId());
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not wrong but seems odd to call getContactId() here since already have the id - this was moved from below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've become rather a fan of using getters as much as possible rather than passing around variables. This is rather in response to the fact that variables get passed around a LOT in civi and they can become quite mystical whereas getters always do the same thing wherever they live (although I have a suspicion this getter might be a bit odd)

@demeritcowboy
Copy link
Contributor

  • [r-run] Issue
    • Editing a relationship using these links changes the other contact to be the logged in user after saving.
    • Editing an employee/r relationship it's missing the type in the dropdown on form load.
    • Recent menu item view link goes to mostly blank page (see inline comment above).

@demeritcowboy
Copy link
Contributor

  • Also when you delete a relationship it loses the context around which contact it was looking at, so it takes you back to the logged in user's contact instead of the one related to the relationship.

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

@colemanw this seems to be pending response from you

@colemanw colemanw marked this pull request as draft August 11, 2021 01:24
@demeritcowboy
Copy link
Contributor

This has merge conflicts and has stalled. I kind of remember this and had a related one of my own somewhere which also stalled.

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