User role homescreen #945
User role homescreen #945
Conversation
This addresses the issue that there’s really nothing (presently) on the homepage.
|
@jkleinsc can you review this PR for the purposes of getting into 1.0? |
There was a problem hiding this comment.
@tangollama there are some code changes needed as I've explained in my comments.
Also, I have added an acceptance test that should pass if you make the suggested changes.
app/mixins/navigation.js
Outdated
| }) | ||
| }), | ||
|
|
||
| findNavItemByRoute: function(route) { |
There was a problem hiding this comment.
This is failing eslint checks. It should be written as:
findNavItemByRoute(route) {
app/mixins/navigation.js
Outdated
| if (el.route == route) { | ||
| return el; | ||
| } else { | ||
| for (var j = 0; j < el.subnav.length; j++) { |
There was a problem hiding this comment.
This line is also failing eslint. Use let instead of var. Actually you should use a foreach instead, eg:
el.subnav.forEach((item) => {
});
app/routes/index.js
Outdated
| @@ -1,6 +1,34 @@ | |||
| import AuthenticatedRouteMixin from 'ember-simple-auth/mixins/authenticated-route-mixin'; | |||
| import Navigation from 'hospitalrun/mixins/navigation'; | |||
| import SetupUserRole from 'hospitalrun/mixins/setup-user-role'; | |||
There was a problem hiding this comment.
This mixin isn't used in this route.
app/routes/index.js
Outdated
| export default Ember.Route.extend(AuthenticatedRouteMixin, Navigation, SetupUserRole, UserRoles, { | ||
| session: inject.service(), | ||
| beforeModel() { | ||
| // this.setupUserRole(); |
There was a problem hiding this comment.
Commented out code should be removed.
app/routes/index.js
Outdated
| let session = this.get('session'); | ||
| if (session != null) { | ||
| let role = session.get('data.authenticated.userRole'); | ||
| if (role != null) { |
There was a problem hiding this comment.
I would use isEmpty instead of != null.
Eg
const { isEmpty } = Ember; <- Declare this after the importsthen in your code:
if (!isEmpty(role)) {
app/mixins/setup-user-role.js
Outdated
| @@ -3,6 +3,7 @@ export default Ember.Mixin.create({ | |||
| setupUserRole() { | |||
| let session = this.get('session'); | |||
| let userRole = session.get('data.authenticated.role'); | |||
| session.set('data.authenticated.userRole', userRole); | |||
There was a problem hiding this comment.
Why not just pull the user's role from data.authenticated.role? Why double store it?
app/mixins/navigation.js
Outdated
| }), | ||
|
|
||
| findNavItemByRoute: function(route) { | ||
| return this.navItems.find(function(el) { |
There was a problem hiding this comment.
I don't think you want to use find here. According to the docs
find "Returns the first item in the array for which the callback returns true". You are not returning true anywhere here and the way this code is written, if the item is found in a subnav item, it is going to return the parent item, not the subnav item. You could accomplish what you need with forEach loops.
app/routes/index.js
Outdated
| // this.setupUserRole(); | ||
| let session = this.get('session'); | ||
| if (session != null) { | ||
| let role = session.get('data.authenticated.userRole'); |
There was a problem hiding this comment.
This should pull data.authenticated.role not data.authenticated.userRole, particularly if you want the acceptance test I added to pass. 😄
Removed commented out code, using isEmpty, removing unused import
Using isEqual and cleaning up the formatting of the looping structure.
Not needed. Thx for the feedback @jkleinsc
|
@jkleinsc I think I've satisfied the requested changes. Happy to discuss the why's Monday. |
|
@jkleinsc All 16 acceptance tests pass for this locally (http://localhost:4200/tests?filter=Testing%20User%20Role%20homescreen). Travis appears to be timing out with a few of these. What shall we do? |
Moved user administrator role test to users-test because it needs the fake rest calls defined there.
@jkleinsc this is for review, not necessarily for merging. The idea here is to overcome the feature issue (and therefore usability concern) of users logging into a back “what do you want to do?” screen. Let’s discuss.