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

432 account overview list details page #605

Merged
merged 37 commits into from Apr 17, 2023

Conversation

nhoening
Copy link
Contributor

@nhoening nhoening commented Mar 9, 2023

  • 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>
@nhoening nhoening linked an issue Mar 9, 2023 that may be closed by this pull request
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@coveralls
Copy link
Collaborator

coveralls commented Mar 9, 2023

Pull Request Test Coverage Report for Build 4720206723

  • 94 of 110 (85.45%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 64.793%

Changes Missing Coverage Covered Lines Changed/Added Lines %
flexmeasures/data/schemas/account.py 15 16 93.75%
flexmeasures/data/services/accounts.py 6 10 60.0%
flexmeasures/ui/crud/accounts.py 23 34 67.65%
Totals Coverage Status
Change from base Build 4700729859: 0.1%
Covered Lines: 7071
Relevant Lines: 10158

💛 - Coveralls

@nhoening nhoening requested a review from GustaafL March 10, 2023 08:58
flexmeasures/api/v3_0/accounts.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/accounts.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/users.py Outdated Show resolved Hide resolved
flexmeasures/data/services/accounts.py Show resolved Hide resolved
GustaafL and others added 3 commits March 15, 2023 09:40
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>
@nhoening nhoening added this to the 0.13.0 milestone Mar 15, 2023
Flix6x
Flix6x previously approved these changes Mar 15, 2023
@Flix6x Flix6x dismissed their stale review March 15, 2023 16:33

I had overlooked additional contributions from Guus added to this PR. Need to review still.

flexmeasures/ui/crud/accounts.py Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/accounts.html Outdated Show resolved Hide resolved
flexmeasures/ui/tests/test_account_crud.py Outdated Show resolved Hide resolved
flexmeasures/ui/tests/utils.py Outdated Show resolved Hide resolved
flexmeasures/ui/tests/utils.py Outdated Show resolved Hide resolved
flexmeasures/ui/tests/test_account_crud.py Show resolved Hide resolved
flexmeasures/ui/tests/test_account_crud.py Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
Copy link
Contributor

@Flix6x Flix6x left a 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.

flexmeasures/api/v3_0/accounts.py Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/accounts.py Show resolved Hide resolved
flexmeasures/ui/crud/accounts.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/accounts.py Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
GustaafL added 2 commits March 21, 2023 13:08
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL GustaafL requested a review from Flix6x March 21, 2023 12:34
Copy link
Contributor

@Flix6x Flix6x left a 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>
@nhoening
Copy link
Contributor Author

@GustaafL I fixed the issue in the API docs, and I created the issue @Flix6x proposed.
The last things to do is changelog entries - it would be good if you can try your hand on those - and then this should be done!

Copy link
Contributor

@Flix6x Flix6x left a 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?

flexmeasures/ui/templates/crud/accounts.html Show resolved Hide resolved
flexmeasures/ui/crud/accounts.py Show resolved Hide resolved
flexmeasures/ui/templates/crud/accounts.html Outdated Show resolved Hide resolved
@nhoening
Copy link
Contributor Author

I wrote in #432:

Also, the main menu entry "accounts" should appear for admins (and link to the account list). The "users" entry should appear for everyone else (as it shows the users in their account).

So that remains a TODO for @GustaafL

Is that agreeable, @Flix6x ?

@Flix6x
Copy link
Contributor

Flix6x commented Mar 25, 2023

Is that agreeable, @Flix6x ?

Yes, with one clarification: the users page should appear for all users, not just for non-admin users. Do you agree?

@nhoening
Copy link
Contributor Author

No, as you should easily reach the users through the account pages.

Signed-off-by: GustaafL <guus@seita.nl>
Copy link
Contributor Author

@nhoening nhoening left a 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 :)

flexmeasures/ui/templates/crud/accounts.html Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/accounts.html Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
GustaafL and others added 4 commits April 4, 2023 15:19
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>
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL GustaafL requested a review from Flix6x April 6, 2023 10:21
Copy link
Contributor

@Flix6x Flix6x left a 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).

documentation/api/change_log.rst Outdated Show resolved Hide resolved
documentation/changelog.rst Outdated Show resolved Hide resolved
flexmeasures/api/v3_0/accounts.py Show resolved Hide resolved
flexmeasures/ui/crud/users.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/users.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/users.py Outdated Show resolved Hide resolved
Signed-off-by: GustaafL <guus@seita.nl>
Copy link
Contributor Author

@nhoening nhoening left a 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.

flexmeasures/ui/templates/defaults.jinja Outdated Show resolved Hide resolved
flexmeasures/ui/templates/defaults.jinja Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/account.html Outdated Show resolved Hide resolved
documentation/api/change_log.rst Outdated Show resolved Hide resolved
flexmeasures/ui/templates/defaults.jinja Outdated Show resolved Hide resolved
Signed-off-by: GustaafL <guus@seita.nl>
@GustaafL GustaafL requested a review from Flix6x April 14, 2023 14:23
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>
Copy link
Contributor

@Flix6x Flix6x left a 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).

@GustaafL GustaafL merged commit 67d2236 into main Apr 17, 2023
5 checks passed
@GustaafL GustaafL deleted the 432-account-overview-list-details-page branch April 17, 2023 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account overview: list & details page
4 participants