-
Notifications
You must be signed in to change notification settings - Fork 48
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ define([ | |
var childComponentsCount = 0; | ||
var initialLeaveRequestAttributes = {}; // used to compare the change in leaverequest in edit mode | ||
var listeners = []; | ||
var loggedInContactId = ''; | ||
var loggedInContact; | ||
var NO_ENTITLEMENT_ERROR = 'No entitlement'; | ||
var role = ''; | ||
var tabs = []; | ||
|
@@ -95,11 +95,11 @@ define([ | |
initAvailableStatusesMatrix(); | ||
initListeners(); | ||
|
||
return loadLoggedInContactId() | ||
return loadLoggedInContact() | ||
.then(initIsSelfRecord) | ||
.then(function () { | ||
return $q.all([ | ||
initRoles(), | ||
initRole(), | ||
loadAbsencePeriods(), | ||
loadStatuses() | ||
]); | ||
|
@@ -248,7 +248,7 @@ define([ | |
* If manager or admin have changed the status through dropdown, assign the same before calling API | ||
*/ | ||
function changeStatusBeforeSave () { | ||
if (vm.isSelfRecord) { | ||
if (vm.isSelfRecord && !vm.isSelfLeaveApprover) { | ||
vm.request.status_id = vm.requestStatuses[sharedSettings.statusNames.awaitingApproval].value; | ||
} else if (vm.canManage) { | ||
vm.request.status_id = vm.newStatusOnSave || vm.request.status_id; | ||
|
@@ -530,7 +530,7 @@ define([ | |
*/ | ||
function initIsSelfRecord () { | ||
var isSectionMyLeave = $rootScope.section === 'my-leave'; | ||
var isMyOwnRequest = +loggedInContactId === +_.get(vm, 'leaveRequest.contact_id'); | ||
var isMyOwnRequest = +loggedInContact.id === +_.get(vm, 'leaveRequest.contact_id'); | ||
var isNewRequest = !_.get(vm, 'leaveRequest.id'); | ||
|
||
vm.isSelfRecord = isSectionMyLeave && (isMyOwnRequest || isNewRequest); | ||
|
@@ -594,7 +594,7 @@ define([ | |
if (vm.request) { | ||
attributes = vm.request.attributes(); | ||
} else if (!vm.canManage) { | ||
attributes = { contact_id: loggedInContactId }; | ||
attributes = { contact_id: loggedInContact.id }; | ||
} | ||
|
||
return attributes; | ||
|
@@ -607,28 +607,51 @@ define([ | |
* | ||
* @return {Promise} | ||
*/ | ||
function initRoles () { | ||
function initRole () { | ||
role = 'staff'; | ||
|
||
// If the user is creating or editing their own leave, they will be | ||
// treated as a staff regardless of their actual role. | ||
if (vm.isSelfRecord) { | ||
return; | ||
} | ||
return (vm.isSelfRecord | ||
? initRoleBasedOnSelfLeaveApproverState() | ||
: initRoleBasedOnPermissions()) | ||
.finally(function () { | ||
vm.canManage = vm.isRole('manager') || vm.isRole('admin'); | ||
}); | ||
} | ||
|
||
/** | ||
* Initiates the user role based on their self leave approver state. | ||
* If the user is creating or editing their own leave request, they will be | ||
* treated as an "admin". | ||
* | ||
* @return {Promise} | ||
*/ | ||
function initRoleBasedOnSelfLeaveApproverState () { | ||
return loggedInContact.checkIfSelfLeaveApprover() | ||
.then(function (isSelfLeaveApprover) { | ||
if (!isSelfLeaveApprover) { | ||
return; | ||
} | ||
|
||
role = 'admin'; | ||
vm.isSelfLeaveApprover = true; | ||
}); | ||
} | ||
|
||
/** | ||
* Initiates user role based on their permissions | ||
* | ||
* @return {Promise} | ||
*/ | ||
function initRoleBasedOnPermissions () { | ||
return checkPermissions(sharedSettings.permissions.admin.administer) | ||
.then(function (isAdmin) { | ||
isAdmin && (role = 'admin'); | ||
}) | ||
.then(function () { | ||
// (role === 'staff') means it is not admin so need to check if manager | ||
return (role === 'staff') && checkPermissions(sharedSettings.permissions.ssp.manage) | ||
.then(function (isManager) { | ||
isManager && (role = 'manager'); | ||
}); | ||
return (role !== 'admin') && checkPermissions(sharedSettings.permissions.ssp.manage); | ||
}) | ||
.finally(function () { | ||
vm.canManage = vm.isRole('manager') || vm.isRole('admin'); | ||
.then(function (isManager) { | ||
isManager && (role = 'manager'); | ||
}); | ||
} | ||
|
||
|
@@ -767,10 +790,11 @@ define([ | |
* | ||
* @return {Promise} | ||
*/ | ||
function loadLoggedInContactId () { | ||
return Session.get().then(function (value) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm... I wonder why linter did not notice this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
loggedInContactId = value.contactId; | ||
}); | ||
function loadLoggedInContact () { | ||
return Contact.getLoggedIn() | ||
.then(function (_loggedInContact_) { | ||
loggedInContact = _loggedInContact_; | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -791,15 +815,12 @@ define([ | |
.then(function (contacts) { | ||
vm.managedContacts = _.remove(contacts.list, function (contact) { | ||
// Removes the admin from the list of contacts | ||
return contact.id !== loggedInContactId; | ||
return contact.id !== loggedInContact.id; | ||
}); | ||
}); | ||
} else { | ||
// In any other case (including managing) | ||
return Contact.find(loggedInContactId) | ||
.then(function (contact) { | ||
return contact.leaveManagees(); | ||
}) | ||
return loggedInContact.leaveManagees() | ||
.then(function (contacts) { | ||
vm.managedContacts = contacts; | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ | |
describe('LeaveRequestCtrl', function () { | ||
var $log, $rootScope, controller, modalInstanceSpy, $scope, $q, dialog, $controller, | ||
$provide, sharedSettings, AbsenceTypeAPI, AbsencePeriodAPI, LeaveRequestInstance, | ||
Contact, ContactAPIMock, EntitlementAPI, LeaveRequestAPI, pubSub, | ||
Contact, ContactInstance, ContactAPIMock, EntitlementAPI, LeaveRequestAPI, pubSub, | ||
requiredTab, WorkPatternAPI, LeaveRequestService; | ||
var role = 'staff'; // change this value to set other roles | ||
|
||
|
@@ -65,7 +65,7 @@ | |
}])); | ||
|
||
beforeEach(inject(function (_$log_, _$controller_, _$rootScope_, _$q_, _dialog_, | ||
_AbsenceTypeAPI_, _AbsencePeriodAPI_, _Contact_, _EntitlementAPI_, _Entitlement_, | ||
_AbsenceTypeAPI_, _AbsencePeriodAPI_, _Contact_, _ContactInstance_, _EntitlementAPI_, _Entitlement_, | ||
_LeaveRequestInstance_, _LeaveRequest_, _LeaveRequestAPI_, _pubSub_, | ||
_WorkPatternAPI_, _LeaveRequestService_) { | ||
$log = _$log_; | ||
|
@@ -75,6 +75,7 @@ | |
dialog = _dialog_; | ||
|
||
Contact = _Contact_; | ||
ContactInstance = _ContactInstance_; | ||
EntitlementAPI = _EntitlementAPI_; | ||
LeaveRequestAPI = _LeaveRequestAPI_; | ||
WorkPatternAPI = _WorkPatternAPI_; | ||
|
@@ -117,6 +118,8 @@ | |
beforeEach(inject(function () { | ||
leaveRequest = LeaveRequestInstance.init(); | ||
leaveRequest.contact_id = CRM.vars.leaveAndAbsences.contactId.toString(); | ||
|
||
spyOn(ContactInstance, 'checkIfSelfLeaveApprover').and.returnValue($q.resolve(false)); | ||
initTestController({ leaveRequest: leaveRequest }); | ||
})); | ||
|
||
|
@@ -576,6 +579,7 @@ | |
|
||
leaveRequest.contact_id = CRM.vars.leaveAndAbsences.contactId.toString(); | ||
$rootScope.section = 'my-leave'; | ||
|
||
initTestController({ leaveRequest: leaveRequest }); | ||
|
||
expectedStatusValue = optionGroupMock.specificValue('hrleaveandabsences_leave_request_status', 'value', '3'); | ||
|
@@ -1342,33 +1346,57 @@ | |
}); | ||
}); | ||
|
||
describe('user edits their own leave request popup', function () { | ||
describe('when user edits their own leave request', function () { | ||
var leaveRequest; | ||
|
||
['staff', 'manager', 'admin'].forEach(function (permissionsRole) { | ||
testRoleForSelfRecord(permissionsRole); | ||
beforeEach(function () { | ||
$rootScope.section = 'my-leave'; | ||
leaveRequest = LeaveRequestInstance.init(); | ||
}); | ||
|
||
/** | ||
* Tests the role for the self record and expects it to be "staff" | ||
* | ||
* @param {String} permissionsRole (staff|manager|admin) | ||
*/ | ||
function testRoleForSelfRecord (permissionsRole) { | ||
describe('when user is ' + permissionsRole, function () { | ||
describe('basic tests', function () { | ||
beforeEach(function () { | ||
spyOn(ContactInstance, 'checkIfSelfLeaveApprover').and.returnValue($q.resolve(false)); | ||
}); | ||
|
||
['staff', 'manager', 'admin'].forEach(function (permissionsRole) { | ||
beforeEach(function () { | ||
$rootScope.section = 'my-leave'; | ||
role = permissionsRole; | ||
leaveRequest = LeaveRequestInstance.init(); | ||
|
||
initTestController({ leaveRequest: leaveRequest }); | ||
}); | ||
|
||
it('sets the staff role', function () { | ||
expect(controller.isRole('staff')).toBeTruthy(); | ||
expect(controller.isRole('staff')).toBe(true); | ||
}); | ||
}); | ||
} | ||
}); | ||
|
||
describe('when user is a self leave approver', function () { | ||
beforeEach(function () { | ||
spyOn(ContactInstance, 'checkIfSelfLeaveApprover').and.returnValue($q.resolve(true)); | ||
}); | ||
|
||
['staff', 'manager', 'admin'].forEach(function (permissionsRole) { | ||
beforeEach(function () { | ||
role = permissionsRole; | ||
|
||
initTestController({ leaveRequest: leaveRequest }); | ||
}); | ||
|
||
it('sets the "admin" role', function () { | ||
expect(controller.isRole('admin')).toBe(true); | ||
}); | ||
|
||
it('sets the `isSelfLeaveApprover` public property to `true`', function () { | ||
expect(controller.isSelfLeaveApprover).toBe(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you need to check if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
}); | ||
|
||
it('sets the `canManage` public property to `true`', function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to check if it's set to false as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
expect(controller.canManage).toBe(true); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('checking if it is a self record', function () { | ||
|
@@ -1399,7 +1427,7 @@ | |
}); | ||
}); | ||
|
||
describe('and the user is checking my own request', function () { | ||
describe('and the user is checking their own leave request', function () { | ||
beforeEach(function () { | ||
leaveRequest.id = _.uniqueId(); | ||
leaveRequest.contact_id = loggedInContactId; | ||
|
@@ -1412,13 +1440,59 @@ | |
}); | ||
}); | ||
|
||
describe('and the user creates a new request for themselves', function () { | ||
beforeEach(function () { | ||
initTestController({ mode: 'create', leaveRequest: leaveRequest }); | ||
describe('and the user creates a new leave request for themselves', function () { | ||
describe('basic tests', function () { | ||
beforeEach(function () { | ||
initTestController({ mode: 'create', leaveRequest: leaveRequest }); | ||
}); | ||
|
||
it('sets is self record as true', function () { | ||
expect(controller.isSelfRecord).toBe(true); | ||
}); | ||
}); | ||
|
||
it('sets is self record as true', function () { | ||
expect(controller.isSelfRecord).toBe(true); | ||
describe('when user is a self leave approver', function () { | ||
beforeEach(function () { | ||
spyOn(ContactInstance, 'checkIfSelfLeaveApprover').and.returnValue($q.resolve(true)); | ||
}); | ||
|
||
['staff', 'manager', 'admin'].forEach(function (permissionsRole) { | ||
beforeEach(function () { | ||
role = permissionsRole; | ||
|
||
initTestController({ mode: 'create', leaveRequest: leaveRequest }); | ||
}); | ||
|
||
it('sets the `isSelfLeaveApprover` public property to `true`', function () { | ||
expect(controller.isSelfLeaveApprover).toBe(true); | ||
}); | ||
|
||
it('sets the `canManage` public property to `true`', function () { | ||
expect(controller.canManage).toBe(true); | ||
}); | ||
|
||
describe('when the user submits a leave request with the "Approved" status', function () { | ||
var approvalStatus; | ||
beforeEach(function () { | ||
approvalStatus = optionGroupMock.specificValue('hrleaveandabsences_leave_request_status', 'value', '1'); | ||
|
||
LeaveRequestAPI.isValid.and.returnValue($q.resolve()); | ||
LeaveRequestAPI.create.and.returnValue($q.resolve({ id: '1' })); | ||
controller.request.status_id = approvalStatus; | ||
|
||
controller.submit(); | ||
$scope.$digest(); | ||
}); | ||
|
||
it('keeps the status unamended', function () { | ||
expect(controller.request.status_id).toEqual(approvalStatus); | ||
}); | ||
|
||
it('calls the corresponding API endpoint', function () { | ||
expect(LeaveRequestAPI.create).toHaveBeenCalled(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
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 theinit
sequence afterinitRole
?There was a problem hiding this comment.
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 tothen