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-4019: Allow self leave approver to select leave request status on My Leave in the Leave Request Modal and delete a leave request #2794

Conversation

igorpavlov-zz
Copy link
Contributor

@igorpavlov-zz igorpavlov-zz commented Jul 26, 2018

Overview

This PR allows self leave approver to select leave request status on My Leave in the Leave Request Modal and delete a leave request. In other words, they become "admins" on My Leave for their own leave requests if they are self leave approvers.

Before

1

After

2

Technical Details

Loading logged in contact instance instead of just their ID

Now we use the Contact.getLoggedIn that resolves with ContactInstance instead of calling Session.get to save some lines of code. We need ContactInstance to call ContactInstance.checkIfSelfLeaveApprover

// var loggedInContactId = '';
var loggedInContact;
// ...

loggedInContact.leaveManagees() // no need to do Contact.find(loggedInContactId) anymore
// ...

loggedInContact.checkIfSelfLeaveApprover() // same as above

Init role now checks if self leave approver

function initRole () {
  role = 'staff';

  return (vm.isSelfRecord
    ? initRoleBasedOnSelfLeaveApproverState()
    : initRoleBasedOnPermissions())

// ...

function initRoleBasedOnSelfLeaveApproverState () {
  return loggedInContact.checkIfSelfLeaveApprover()
    .then(function (isSelfLeaveApprover) {
      if (!isSelfLeaveApprover) {
        return;

      role = 'admin';
      vm.isSelfLeaveApprover = true;

// ....

function initCanManage () { // used in the init chain
  vm.canManage = vm.isRole('manager') || vm.isRole('admin');
}

Why role = 'admin'?

From the business logic perspective a self leave approver "administers" their own leave requests.

Why vm.isSelfLeaveApprover = true?

This is a new public property because we want to show some specific UI elements to the self leave approver: Status Selector and Delete button. We don't want to show other admin UI elements, such as, for example, the name of the contact on the top of the modal.

We also cannot only use vm.canManage property alone, because the templates also consider vm.isSelfRecord which prevents some UI elements to be shown when creating/editing you own leave request.

The solution is to use a combination in the templates:

&& !$ctrl.isSelfRecord
// becomes
&& (!$ctrl.isSelfRecord || $ctrl.isSelfLeaveApprover)

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

@igorpavlov-zz igorpavlov-zz changed the title PCHR-4019: Allow self leave approver to select leave request status in the Leave Request Modal and delete a leave request PCHR-4019: Allow self leave approver to select leave request status on My Leave in the Leave Request Modal and delete a leave request Jul 26, 2018
@igorpavlov-zz igorpavlov-zz force-pushed the PCHR-4019-allow-self-approver-to-select-lr-status branch 2 times, most recently from 0dab694 to ca66f97 Compare July 26, 2018 12:42
@igorpavlov-zz igorpavlov-zz force-pushed the PCHR-4019-allow-self-approver-to-select-lr-status branch from ca66f97 to de59269 Compare July 26, 2018 12:43
? initRoleBasedOnSelfLeaveApproverState()
: initRoleBasedOnPermissions())
.finally(function () {
vm.canManage = vm.isRole('manager') || vm.isRole('admin');
Copy link
Member

Choose a reason for hiding this comment

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

can we remove this from here, move it to a initCanManage function, and call that function from the init sequence after initRole?

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. I also checked if we need finally, I don't think so, so I changed to then

@@ -767,10 +790,11 @@ define([
*
* @return {Promise}
*/
function loadLoggedInContactId () {
return Session.get().then(function (value) {
Copy link
Member

Choose a reason for hiding this comment

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

since we now don't use Session, can you please remove any reference to it from this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... I wonder why linter did not notice this.

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.

Copy link
Member

Choose a reason for hiding this comment

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

The linter doesn't care if you don't use one of the parameters in your function. I guess it does that in case you need to use the third parameter, but not the second one.

});

it('sets the `isSelfLeaveApprover` public property to `true`', function () {
expect(controller.isSelfLeaveApprover).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

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

you need to check if isSelfLeaveApprover is set to false in the basic tests group`.

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.

expect(controller.isSelfLeaveApprover).toBe(true);
});

it('sets the `canManage` public property to `true`', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Need to check if it's set to false as well.

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.

@@ -507,6 +508,13 @@ define([
].concat(defaultStatuses);
}

/**
* Defines if the contact ca manage the leave request
Copy link
Member

Choose a reason for hiding this comment

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

*can

@igorpavlov-zz igorpavlov-zz force-pushed the PCHR-4019-allow-self-approver-to-select-lr-status branch from c290da6 to 1438816 Compare July 27, 2018 11:20
@igorpavlov-zz igorpavlov-zz merged commit 7768069 into PCHR-3997-allow-own-leave-approver Jul 27, 2018
@igorpavlov-zz igorpavlov-zz deleted the PCHR-4019-allow-self-approver-to-select-lr-status branch July 27, 2018 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants