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-2395: Fix admin self leave request popup #1999

Merged

Conversation

igorpavlov-zz
Copy link
Contributor

@igorpavlov-zz igorpavlov-zz commented Jul 13, 2017

Overview

There are some issues with the Leave Request popup:

  1. On My Leave page it looks and behaves the same as on Manager Leave or Admin Dashboard page.
  2. The popup flickers during loading.
  3. The popup allows admins/managers to choose themselves in the dropdown when recording on behalf.

This PR fixes these issues.

Before

new

After

new

Technical Details

Earlier in order to check if you record for yourself the system was checking if you are Staff. This is logically wrong, since Admin or Manager can also record for themselves via My Leave page, but they are not Staff.

  1. A new flag was introduced: this.isSelfRecord. If this param is TRUE it indicates that you create/edit a request for yourself. For example:
<a
  leave-request-popup
  contact-id="vm.contactId"
  is-self-record="true">
</a>
  1. Currently logged in admin is removed from the their managees list.

  2. The flickering is fixed by adding an additional condition to HTML.

Comments

  1. The Absence New Record dropdown now can also pass this.isSelfRecord through itself.

  • Tests Pass

@igorpavlov-zz igorpavlov-zz changed the base branch from staging to la-milestone-3 July 13, 2017 13:08
@igorpavlov-zz igorpavlov-zz force-pushed the PCHR-2395-fix-admin-self-leave-request-popup branch 7 times, most recently from df49841 to c6a2944 Compare July 13, 2017 15:25
@igorpavlov-zz igorpavlov-zz force-pushed the PCHR-2395-fix-admin-self-leave-request-popup branch from c6a2944 to 7a7ef9a Compare July 13, 2017 15:33
Copy link
Member

@reneolivo reneolivo left a comment

Choose a reason for hiding this comment

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

initTestController({
  contactId: leaveRequest.contact_id, // staff's contact id
  isSelfRecord: true,
  leaveRequest: leaveRequest
});

I see spec files have been amended to include the isSelfRecord: true option, but are there any note worthy test cases for isSelfRecord: false?

this.managedContacts = _.remove(contacts.list, function (contact) {
// Removes the manager/admin from the list of managees
return contact.id !== self.directiveOptions.contactId;
});
Copy link
Member

Choose a reason for hiding this comment

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

}.bind(this)); is being used above, it should be used here instead of var self = this, etc.

@igorpavlov-zz
Copy link
Contributor Author

igorpavlov-zz commented Jul 13, 2017

I see spec files have been amended to include the isSelfRecord: true option, but are there any note worthy test cases for isSelfRecord: false?

Any place in tests withdirectiveOptions that does not contain isSelfRecord tests what happens if isSelfRecord is false. And since isSelfRecord is a binding, it only can be true of false (even if not passed at all).

var directiveOptions = {
  contactId: adminID, // admin's ID
  ... // not specifying isSelfRecord tests if it is false
};

However, I added a test to make sure Admin does not appear in the list of managees.

@igorpavlov-zz igorpavlov-zz force-pushed the PCHR-2395-fix-admin-self-leave-request-popup branch from c0ddb3e to 56a3774 Compare July 13, 2017 16:27
});

it('loads exactly one contact', function () {
expect(_.find($ctrl.managedContacts, { 'id': adminId })).toBeUndefined();
Copy link
Member

Choose a reason for hiding this comment

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

if the statement is it loads exactly one contact shouldn't the test be something like expect($ctrl.managedContacts.length).toBe(1) or maybe the statement needs to change to something like it does not include admin on contact list ?

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.

@igorpavlov-zz igorpavlov-zz force-pushed the PCHR-2395-fix-admin-self-leave-request-popup branch from 56a3774 to 726cf8a Compare July 13, 2017 16:37
@igorpavlov-zz igorpavlov-zz merged commit 17d0009 into la-milestone-3 Jul 13, 2017
@igorpavlov-zz igorpavlov-zz deleted the PCHR-2395-fix-admin-self-leave-request-popup branch July 13, 2017 16:40
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