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

routes/turnilo: allow query params on GET dashs #2

Merged
merged 7 commits into from
Jun 14, 2024
Merged

Conversation

msune
Copy link
Member

@msune msune commented Jun 12, 2024

routes/turnilo: allow query params on GET dashs

Allow the following query parameters (exact match) on the main GET dashboards URL:

* `shortName`
* `dataCube`

This allows clients to validate whether a unique constrain shortName+dataCube is in use and by which dashboard ID.

Added test coverage too.

This commit moves turnilo dashboard routes to a dedicated module.
@msune msune force-pushed the turnilo_get_filter branch from 3aae052 to 8960286 Compare June 12, 2024 21:09
Allow the following query parameters (exact match) on the main
GET dashboards URL:

* `shortName`
* `dataCube`

This allows clients to validate whether a unique constrain
`shortName+dataCube` is in use and by which dashboard ID.

Added test coverage too.
@msune msune force-pushed the turnilo_get_filter branch 2 times, most recently from a0830e9 to 483acf0 Compare June 12, 2024 22:04
msune added 3 commits June 13, 2024 00:06
Bump dependencies to latest, to fix an invalid Warning by sqlalchemy
Pass -Werror to pytest so that warnings become errors.
This commit:

* Cleans up some left overs from the transition of SQLAlchemy ->
  SQLModel
* Fixes annoying deprecation SQLModel warnings
@msune msune force-pushed the turnilo_get_filter branch from 483acf0 to 3505ac0 Compare June 12, 2024 22:06
@msune
Copy link
Member Author

msune commented Jun 12, 2024

I think you will like what I piled on top @OscarMoya (yeah, I know, I should have stubbed this in another PR, but there were some conflicts):

commit 3505ac01085c44574dc2977a67dff3453b0af626 (HEAD -> turnilo_get_filter, origin/turnilo_get_filter)
Author: Marc Sune <[email protected]>
Date:   Wed Jun 12 23:48:23 2024 +0200

    services: fix leftovers alchemy->sqlmodel & warns
    
    This commit:
    
    * Cleans up some left overs from the transition of SQLAlchemy ->
      SQLModel
    * Fixes annoying deprecation SQLModel warnings

commit 086c861907c14bc72aaa6fd3bd548234537001a2
Author: Marc Sune <[email protected]>
Date:   Wed Jun 12 23:55:00 2024 +0200

    test: turn warning as errors
    
    Pass -Werror to pytest so that warnings become errors.

commit 0285f21652d9bd464aaae6c73f1791a42f492bc4
Author: Marc Sune <[email protected]>
Date:   Wed Jun 12 23:22:55 2024 +0200

    requirements.txt: bump deps
    
    Bump dependencies to latest, to fix an invalid Warning by sqlalchemy

Copy link

@OscarMoya OscarMoya left a comment

Choose a reason for hiding this comment

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

If I were you, I would consider adopt some of my proposals to make sure the code is secure and extensible with minimal boilerplate, but as it is right now it would work

src/routes/turnilo_dashboard_routes.py Outdated Show resolved Hide resolved
src/routes/turnilo_dashboard_routes.py Outdated Show resolved Hide resolved
msune added 2 commits June 14, 2024 00:04
Make it easy to run within a venv.
Adopt Oscar's suggestion to use a Query class to validate query
parameters.

Unfortunately, due to the circular dependency, the Query class
is moved to services (due to typing).

Signed-off-by: Marc Sune <[email protected]>
Signed-off-by: Oscar Moya <[email protected]>
Copy link

@OscarMoya OscarMoya left a comment

Choose a reason for hiding this comment

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

LGTM

@msune msune merged commit 9376293 into main Jun 14, 2024
8 checks passed
@msune msune deleted the turnilo_get_filter branch June 14, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants