Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

add cashier role in roles #1027

Merged
merged 4 commits into from
Apr 12, 2017
Merged

Conversation

baoqchau
Copy link
Contributor

@baoqchau baoqchau commented Mar 25, 2017

Fixes #303
Changes proposed in this pull request:

  • Add "cashier" roles in list of roles
  • Test user with "cashier" role to see if the left navigation only display what the cashier can do.
  • Still need to work on left navigation so that the sub-nav should only display only 3 sub-navs: Invoice, New invoice and Cashier screen(not finish)
    screenshot_20170325_155832

cc @HospitalRun/core-maintainers

@baoqchau
Copy link
Contributor Author

@jkleinsc Should I create additional capabilities for Prices and Price Profile because I think that a cashier does not need access to Price and Price Profile right?

@jkleinsc
Copy link
Member

@baoqchau I took a look and you are correct that Prices and Price Profile should use a different capability. Looking at the current capabilities, there is one called pricing that should be used for Prices and Price Profile, so you don't need to add a new one, you just need to modify the navigation to use pricing instead of invoices.

@jkleinsc
Copy link
Member

@baoqchau I'm not sure where you are with this feature work, but I did want to ask that you add an acceptance test for the new functionality. You may have already been planning on this, but I just thought I would ask. Having the acceptance tests from the beginning of a feature is really helpful.

@baoqchau
Copy link
Contributor Author

Hi @jkleinsc. Thanks for reminding me about the acceptance test. What I plan to do in this PR is adding new "cashier" roles and create a basic cashier screen with a "Cashier" sub-navigation on the left navigation bar. The acceptance test will test that a user with "cashier" roles will have access only to appointments and 3 sub-nav in the "invoices" tab, like in this picture. I will do other parts in upcoming PR. Is this plan good to you?
screenshot_20170329_081939

@jkleinsc
Copy link
Member

@baoqchau sounds good to me!

@baoqchau
Copy link
Contributor Author

baoqchau commented Mar 29, 2017

@jkleinsc should I create additional roles(i.e "cashier" role) in helpers/authenticate-user.js or I should just use the default authenticateUser() one? Because if we test with the System Administrator user, the left navigation bar will have 5 sub-navigation, including the Cashier screen

@baoqchau
Copy link
Contributor Author

baoqchau commented Mar 30, 2017

In addition, should the cashier have access to Paid invoices? If so, the cashier screen will look just like the Invoices index page, and the different is just the capabilities of the cashier (i.e buttons on the right). If not, we need to limit the user access to Paid invoices by adding authentication in invoices/route.js

@jkleinsc
Copy link
Member

@baoqchau as far as testing is concerned, you can pass in the role to authenticateUser, eg:

      authenticateUser({
        name: '[email protected]',
        roles: ['Cashier', 'user'],
        role: 'Cashier',
        prefix: 'p1'
      });

As far as paid invoices, the cashier should not have access to them.

@baoqchau
Copy link
Contributor Author

baoqchau commented Apr 9, 2017

Ok so what I found out is that after my cashier-role test in invoices-test, even though I have tried to invalidateSession() after my cashier-role test (not on the latest commit but on my local machine), the authenticateUser in the next test still not work, which leads to all tests after cashier-role test failed. How should i solve this?

@jkleinsc
Copy link
Member

@baoqchau I'm not sure why this happening. I pulled down your code and see the same issue. You shouldn't need to call invalidateSession() though.

@jkleinsc
Copy link
Member

@baoqchau I looked a little further into this, and when I ran the tests in the browser (eg run ember serve and then go to http://localhost:4200/tests), it appears that the navigation is throwing some errors. There is a small fix to the navigation that will correct these errors and allow the tests to pass. I am going to update master with that change and then you can merge master into your fork.

jkleinsc added a commit that referenced this pull request Apr 11, 2017
Resolves issue encountered in #1027 during testing.
@jkleinsc
Copy link
Member

@baoqchau I just updated master with a fix for the error you were encountering in your testing, so if you get the latest it should resolve the issue.

…rontend into cashier-role

update navigation components to fix bugs in CI
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for sticking with this PR @baoqchau!

@jkleinsc jkleinsc merged commit 6d0b1d8 into HospitalRun:master Apr 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants