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

PCHR-3625: Workflow creation, prompt user without manager #351

Merged

Conversation

reneolivo
Copy link
Member

@reneolivo reneolivo commented Apr 26, 2018

Overview

This PR displays a warning message when creating workflows and the target contact is missing a relationship that was to be selected by default. It also allows the user to create this relationship from within the workflow creation modal.

⚠️ This PR depends on the following PRs:

Before

screenshot 2018-04-26 03 50 26
The target contact is missing a relationship "HR Manager is" for the "Revoke Access to Database" activity. The assignee is shown as blank, but no warning is displayed.

After

anim

Technical Details

The initDefaultAssigneesForActivities method was refactored so the sequence changed to first get the complete list of activities and parent activity types, then load and assign the default assignees, and finally check if any of the default assignees by relationships were not found and display warnings:

function initDefaultAssigneesForActivities () {
  // skip if an activity set and types have not been defined:
  if (!vm.activity.activitySet.activityTypes) {
    return $q.resolve();
  }

  // this object holds the activity types with missing relationships and the relationship type that is missing.
  // here we are resetting it:
  vm.relationshipMissingWarnings = {};

  return $q.resolve(getActivitiesAndTypes())
    .then(setDefaultAssignees)
    .then(setRelationshipMissingWarnings);
}

getActivitiesAndTypes returns the whole list of activities and their related activity type:

function getActivitiesAndTypes () {
  var allActivities = _.chain(vm.documentList).concat(vm.taskList).value();
  var activityAndTypes = vm.activity.activitySet.activityTypes.map(function (activityType) {
    var activity = _.find(allActivities, { name: activityType.name });

    if (activity) {
      return {
        activity: activity,
        type: activityType
      };
    }
  });

  // removes empty values:
  return _.compact(activityAndTypes);
}

setDefaultAssignees gets the default assignees for the activity type and stores them in the activity (if found):

function setDefaultAssignees (activitiesAndTypes) {
  var promises = activitiesAndTypes.map(function (activityAndType) {
    return getDefaultAssigneeForActivityType(activityAndType.type)
      .then(function (assigneeId) {
        activityAndType.activity.assignee_contact_id = assigneeId;

        return activityAndType;
      });
  });

  return $q.all(promises);
}

setRelationshipMissingWarnings checks if there are missing relationships and setups the relationshipMissingWarnings object:

function setRelationshipMissingWarnings (activitiesAndTypes) {
  var activitiesWithMissingRelationships;

  // skip if no contact has been selected:
  if (!vm.assignment.client_id) {
    return;
  }

  activitiesWithMissingRelationships = getActivitiesWithoutDefaultRelationship(activitiesAndTypes);
  vm.relationshipMissingWarnings = _.chain(activitiesWithMissingRelationships)
    .indexBy('activity.activity_type_id').mapValues('type.default_assignee_relationship').value();
}

function getActivitiesWithoutDefaultRelationship (activitiesAndTypes) {
  return activitiesAndTypes.filter(function (activityAndType) {
    var isDefaultAssigneeByRelationship = activityAndType.type.default_assignee_type === defaultAssigneeOptionsIndex.BY_RELATIONSHIP;
    var hasNoAssignee = _.isEmpty(activityAndType.activity.assignee_contact_id);

    return isDefaultAssigneeByRelationship && hasNoAssignee;
  });
}

Finally, the user can create the relationship by pressing a button that triggers the createRelationshipForTargetContact method:

function createRelationshipForTargetContact () {
  var formUrl = CRM.url('civicrm/contact/view/rel', {
    action: 'add',
    cid: vm.assignment.client_id
  });

  CRM.loadForm(formUrl)
    .on('crmFormSuccess', function () {
      // $evalAsync will execute the code in the next digest.
      // this is done because CRM.loadForm works outside of Angular:
      vm.$evalAsync(function () {
        // tells the Relationship model to request relationships without using the cache.
        canCacheRelationshipRequests = false;

        initDefaultAssigneesForActivities()
          .then(loadAndCacheContactForActivities);
      });
    });
}

@reneolivo reneolivo changed the title Pchr 3625 prompt user without manager PCHR-3625: Workflow creation, prompt user without manager Apr 26, 2018
@reneolivo reneolivo force-pushed the PCHR-3625-prompt-user-without-manager branch from 375e26b to 7f8e6aa Compare April 26, 2018 08:09
@reneolivo
Copy link
Member Author

@jamienovick can you please have a look at the result? I didn't have a mock, but I'm supposing it was more or less something like this.

cid: vm.assignment.client_id
});

CRM.loadForm(formUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a service in L&A Reqangular for that, we may want to have the same service in T&A?

cc @AkA84

Copy link
Member Author

Choose a reason for hiding this comment

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

I could use the loadForm from reqangular, but there are some issues:

  • I also need CRM.url which is not in reqangular.
  • The reqangular one also suffers from the digest issue, so it needs to be augmented.

In short, there's no clear benefit on using the reqangular wrapper, the CRM variable can be accessed without issues from tests and all of its services can be used directly without having to wrap them and add more tests on top of them.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think leave it as is.

/**
* Returns object pairings of all the activities and their parent activity type.
*
* @return {Array} Each elements contains an object that holds an activity and
Copy link
Contributor

Choose a reason for hiding this comment

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

Each element

}
});

// removes empty values:
Copy link
Contributor

Choose a reason for hiding this comment

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

No need in this comment, compact is pretty self explanatory.

* Returns all activites that are missing a default assignee related to the
* target contact.
*
* @param {Array} activitiesAndTypes - An array of activities and their types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please align params and return.

* Finds the default assignee for the given activity type and assigns them
* to their corresponding activity.
*
* @param {Array} activitiesAndTypes - An array of activities and their types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please align params and return.

}

/**
* It populates the `relationshipMissingWarnings` object based on activites
Copy link
Contributor

Choose a reason for hiding this comment

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

Populates the... (no need in "it")

@reneolivo
Copy link
Member Author

When creating a new relationship for the target contact we use CRM.loadForm. Since CRM.loadForm is a jQuery/vanilla js function it needs to use a $timeout so the result is included in the next angular digest cycle. If this is not added the new contact relation name would not be displayed in the dropdown unless the user manually interacts with the form.

I have tested the $timeout manually to make sure this works.

@reneolivo reneolivo force-pushed the PCHR-3369-tasks-and-workflows-ux-improvements branch from adc40b5 to 56b875d Compare April 30, 2018 12:23
@reneolivo reneolivo changed the base branch from PCHR-3369-tasks-and-workflows-ux-improvements to PCHR-3369-tw-ux-improvements August 12, 2018 17:59
@reneolivo reneolivo force-pushed the PCHR-3625-prompt-user-without-manager branch from 2a51b9d to f604c76 Compare August 17, 2018 08:57
@reneolivo reneolivo force-pushed the PCHR-3625-prompt-user-without-manager branch from f604c76 to 2296405 Compare August 17, 2018 09:07
@reneolivo
Copy link
Member Author

Changes to the PR starting from commit PCHR-3625: Fix typo:

Overview

These commits add the following changes to the PR:

  • It fixes a typo in the warning message.
  • Follows the new structure for the default_assignee_relationship field. Before the field would simply store the ID of the relationship type and a single direction would be considered, but the structure of the field changed to also contain the direction of the relationship. For example, the field value could be 123_b_a where 123 is the relationship type's id, and b_a means the field to query is contact_id_b using the client id and return contact_id_a for the default assignee.

Before

pr-before

After

pr-after

Technical details

Relationship types are now loaded on initialization of the modal component for two reasons:

  1. to be able to display the tooltip warning for a missing relationship type.
  2. to determine if the relationship is bidirectional and correctly prepare the filters for the relationship type model.
var relationshipTypesCache = {};

(function init () {
  vm.loading.component = true;

  initRelationshipTypesCache()
    .then(initDefaultAssigneeOptionsIndex)
    .then(initWatchers)
    .finally(function () {
      vm.loading.component = false;
    });
}());

function initRelationshipTypesCache () {
  return RelationshipType.all({ options: { limit: 0 } })
    .then(function (relationshipTypes) {
      relationshipTypesCache = _.indexBy(relationshipTypes.list, 'id');
    });
}

An important function introduced in these commits is getRelationshipTypeDetails which given a default assignee relationship value like 123_b_a, it returns important information about the relationship type, including the id, if the relationship is bidirectional or not, the source and target contact field names, and the label for the relationship, which depends on the direction:

function getRelationshipTypeDetails (defaultAssigneeRelationship) {
  var relationshipTypeRules = defaultAssigneeRelationship.split('_');
  var relationshipTypeId = relationshipTypeRules[0];
  var relationshipTypeDirection = relationshipTypeRules[1];
  var relationshipType = relationshipTypesCache[relationshipTypeId];
  var isRelationshipTypeBidirectional = relationshipType.label_a_b === relationshipType.label_b_a;
  var relationshipLabel = isRelationshipTypeBidirectional || relationshipTypeDirection === 'a'
    ? relationshipType.label_a_b
    : relationshipType.label_b_a;

  return {
    id: relationshipType.id,
    isBidirectional: isRelationshipTypeBidirectional,
    sourceContactFieldName: 'contact_id_' + relationshipTypeRules[1],
    targetContactFieldName: 'contact_id_' + relationshipTypeRules[2],
    label: relationshipLabel
  };
}

To determine if a relationship type is bidirectional we need to compare their label_a_b and label_b_a to see if they are similar. Please see the comments section of this PR #408 if you have doubts about this approach. TL;DR: That's how it is done in core and we can't change it unless we change several things in core.

Displaying the relationship type label has now been modified so it works like this:

function setRelationshipMissingWarnings (activitiesAndTypes) {
  var activitiesWithMissingRelationships;

  // skip if no contact has been selected:
  if (!vm.assignment.client_id) {
    return;
  }

  activitiesWithMissingRelationships = getActivitiesWithoutDefaultRelationship(activitiesAndTypes);
  vm.relationshipMissingWarnings = _.chain(activitiesWithMissingRelationships)
    .map(function (activityAndType) {
      var relationshipTypeDetails = getRelationshipTypeDetails(
        activityAndType.type.default_assignee_relationship);

      return {
        activity_type_id: activityAndType.activity.activity_type_id,
        relationshipLabel: relationshipTypeDetails.label
      };
    })
    .indexBy('activity_type_id')
    .mapValues('relationshipLabel')
    .value();
}

This will return an object similar to:

{
  "456": "HR Manager is",
  "689": "Spouse of"
}

Where 456 and 689 are the ids of the activity type, which are used on the view to match if a specific activity is missing their default assignee by relationship. (this last part has not changed from the original PR).

To query the default assignee using the relationship model we now need to know if the relationship type is bidirectional in order to change the filters. Here are some scenarios and their expected filters (simplified):

// when the value is 123_a_b:
{
  relationship_type_id: 123,
  contact_id_a: targetContact.id
}

// when the value is 123_b_a:
{
  relationship_type_id: 123,
  contact_id_b: targetContact.id
}

// when the relationship type is bidirectional and the value is 123_a_b:
{
  relationship_type_id: 123,
  contact_id_a: targetContact.id,
  contact_id_b: targetContact.id,
  options: { or: [['contact_id_a', 'contact_id_b']] }
}
// this last one means "where the target contact is in either side of the relationship".

The actual code looks like this:

function getDefaultAssigneeForActivityTypeByRelationship (activityType) {
  // ...

  relationshipTypeDetails = getRelationshipTypeDetails(activityType.default_assignee_relationship);
  filters = getDefaultAssigneeFiltersForRelationshipType(relationshipTypeDetails);

  return Relationship.allValid(filters, null, null, canCacheRelationshipRequests)
    .then(function (result) {
      canCacheRelationshipRequests = true;

      return result.list.map(function (relationship) {
        if (relationshipTypeDetails.isBidirectional) {
          // depending on the position of the target contact we return the opposite contact id:
          return relationship.contact_id_a === vm.assignment.client_id
            ? relationship.contact_id_b
            : relationship.contact_id_a;
        } else {
          return relationship[relationshipTypeDetails.targetContactFieldName];
        }
      });
    });
}

function getDefaultAssigneeFiltersForRelationshipType (relationshipTypeDetails) {
  var filters, sourceContactFieldName;

  if (relationshipTypeDetails.isBidirectional) {
    return {
      relationship_type_id: relationshipTypeDetails.id,
      contact_id_a: vm.assignment.client_id,
      contact_id_b: vm.assignment.client_id,
      options: {
        or: [['contact_id_a', 'contact_id_b']],
        limit: 1
      }
    };
  } else {
    sourceContactFieldName = relationshipTypeDetails.sourceContactFieldName;
    filters = {
      relationship_type_id: relationshipTypeDetails.id,
      options: { limit: 1 }
    };
    // depending on the direction we can filter by either `contact_id_a` or `contact_id_b`:
    filters[sourceContactFieldName] = vm.assignment.client_id;

    return filters;
  }
}

✅Manual Tests - done
✅Jasmine Tests - included and passed
✅JS distributive files - included
⏹CSS distributive files - not needed
⏹Backstop tests - not needed
⏹PHP Unit Tests - not needed

scope.assignment.client_id = _.uniqueId();
scope.assignment.case_type_id = selectedAssignment.id;
scope.activity.activitySet = selectedAssignment.definition.activitySets[0];
}

/**
* Initializes the cache for assignment and document types.
* Initializes the cache for assignment, tasks, and document types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document why we are slicing arrays in half, this may be a bit difficult to understand without an explanation.

@reneolivo reneolivo merged commit ed375d4 into PCHR-3369-tw-ux-improvements Aug 17, 2018
@reneolivo reneolivo deleted the PCHR-3625-prompt-user-without-manager branch August 17, 2018 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants