-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement submission statistics & basic RBAC #23
Conversation
Code Climate has analyzed commit 977712a and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
fc0296c
to
ed9c95a
Compare
This is great! The table is a little wide - suggest we shrink the last two columns as wide as the column titles, first column as wide as the longest value, and squish/center it - this will buy us some blank spaces on the left and right ends outside the table to ultimately make it more readable. if you're making it responsive we could do the above, then resizing the browser width could first keep the table the same width while reducing the blank side space, then once the table is as wide as a more narrow browser, go into the standard approach for shrinking as done elsewhere. The last comment is numbers in tables are typically right-aligned, that might look weird until we shrink down the table but I would suggest doing that as well. Also since I couldn't test with a sample navigator, I am making the assumption that the navigator filter works. If any of this is unclear please contact me! Just |
Hi Kyle! I think the stats page look great! It's simple and straightforward 👍 First, the drop down calendar box isn't aligned with the text (submission by range/ month/ submissions by assigned navigator). But, I realized it gets aligned only when you select something from the drop down. I can see the boxes shifting position as I select a date. It'd be great if you could fix the position, so that it's aligned from the beginning and doesn't move. Another minor thing, it would be great if we could put some contrast between the statistics heading/ title vs. the actual state. Example: [Submissions by intake state (as it is) : Assigned (in bold letters)] or the other way around. This will increase the readability for the users and help them visually capture the necessary info faster :) This is my first review comment, so please let me know if you need clarification on what I'm saying. Thank you! |
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.
managed to understand it.. interesting approach
uuidValidate(statisticFilters.value.userId as string) && | ||
uuidVersion(statisticFilters.value.userId as string) === 4); | ||
|
||
if (valid) { |
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.
i wonder if you need this valid check.
maybe userId will either be empty or a uuid.. both work in the query.(?)
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.
userId is a uuid once a user has been selected from the drop down. Up until that point it is filled with whatever the user is typing into the box. Kind of annoying how it works.
New db function, new tab on submissions page for statistics
Filters were not supposed to be additive
Also incl bug fix for loading permit data from older intake versions
e358234
to
68006f3
Compare
count(*) filter (where s."applicationStatus" = 'NEW'), | ||
count(*) filter (where s."applicationStatus" = 'IN PROGRESS'), | ||
count(*) filter (where s."applicationStatus" = 'DELAYED'), | ||
count(*) filter (where s."applicationStatus" = 'COMPLETED'), |
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.
Intake status and application status need to be title case and not upper case.
Forbid front end navigation and API access if at least one role isn't present
e7c5c91
to
977712a
Compare
Description
Types of changes
New feature (non-breaking change which adds functionality) -->
Checklist
Further comments