-
Notifications
You must be signed in to change notification settings - Fork 35
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
432 account overview list details page #605
Conversation
nhoening
commented
Mar 9, 2023
•
edited
Loading
edited
- Accounts API
- Small improvements: Health API documented, user.last_seen in CLI account overview
- Account list and details page
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Pull Request Test Coverage Report for Build 4720206723
💛 - Coveralls |
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
…ed for filtering Signed-off-by: Nicolas Höning <nicolas@seita.nl>
I had overlooked additional contributions from Guus added to this PR. Need to review still.
Signed-off-by: GustaafL <guus@seita.nl>
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.
Thanks for revising. Only a few small changes left, and one idea for a follow-up ticket.
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: GustaafL <guus@seita.nl>
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.
Thanks for implementing the revisions. Three things left that I see:
- Docstring of account index API (see unresolved comment)
- Missing changelog entries (general changelog and API changelog); we indicate this with a label on the PR, feel free to remove the label after pushing a changelog entry
- Can you create the follow-up ticket I mentioned? (see unresolved comment)
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
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.
Thanks for addressing my comments. I've now moved on from a line-by-line review to reviewing the new functionality (by checking out the branch, running FlexMeasures and running through the new pages manually). I saw some missing styling and missing auth decorators.
@nhoening The accounts page is not more informative than the user's own account page, in case the user does not have read access to multiple accounts. Should we hide the menu link for such (non-admin) users?
Yes, with one clarification: the users page should appear for all users, not just for non-admin users. Do you agree? |
No, as you should easily reach the users through the account pages. |
Signed-off-by: GustaafL <guus@seita.nl>
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 found a few improvements while trying out the new UI :)
Signed-off-by: GustaafL <guus@seita.nl>
* Add AccountRoleSchema and connect it to the AccountSchema Signed-off-by: F.N. Claessen <felix@seita.nl> * Update test Signed-off-by: F.N. Claessen <felix@seita.nl> --------- Signed-off-by: F.N. Claessen <felix@seita.nl> Co-authored-by: Flix6x <flix6x@users.noreply.github.com>
Signed-off-by: GustaafL <guus@seita.nl>
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 have some other comments, but those concern the front-end UX so it will be easier to go through those in person (video call).
Signed-off-by: GustaafL <guus@seita.nl>
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 have a few comments about the menu and I'd like to list public assets on their own account page.
Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…list-details-page # Conflicts: # documentation/changelog.rst # flexmeasures/ui/templates/crud/users.html
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
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.
Thanks for amending! NB I fixed a few more issues I found, including the merge conflict with PR #634 (I also copyied the fix in that PR to our new copy of the users html table).