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

feat(ui-api, digest): enhance DigestDialog and add dedicated endpoint for digest UI #2240

Merged
merged 30 commits into from
Feb 10, 2025

Conversation

MartinBelthle
Copy link
Contributor

@MartinBelthle MartinBelthle commented Nov 21, 2024

Fix [ANT-2449] for the back-end part

Endpoint: GET "/private/studies/{study_id}/outputs/{output_id}/digest-ui"

ResponseModel:

class DigestMatrixUI(AntaresBaseModel):
    rowHeaders: t.List[str]
    columns: t.List[str]
    data: t.List[t.List[str]]
    grouped_columns: bool


class DigestUI(AntaresBaseModel):
    area: DigestMatrixUI
    districts: DigestMatrixUI
    flow_linear: DigestMatrixUI
    flow_quadratic: DigestMatrixUI

Copy link
Member

@sylvlecl sylvlecl left a comment

Choose a reason for hiding this comment

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

My interrogations:
we should chose between 2 things for this new endpoint:

  • is it only UI related ? in this case we could stick with the "poor" table representation, but we should make it more obvious in the path and in the class names
  • is it meant to become an API endpoint ? in that case we need to have a more specific model with better names and probably another structure (not tables everywhere)

from antarest.study.storage.rawstudy.model.filesystem.root.output.simulation.mode.mcall.synthesis import OutputSynthesis


class DigestMatrixDTO(AntaresBaseModel):
Copy link
Member

Choose a reason for hiding this comment

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

DTOs are really front-facing classes which should be close to the web layer, not deep inside the filesystem layer.
We should at least rename them here to not have DTO in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I renamed it DigestUI to fit with what we said. But how do you picture the class not being here ?

"/private/studies/{study_id}/outputs/{output_id}/digest",
tags=[APITag.study_outputs],
summary="Get an output digest file",
response_model=DigestDTO,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You talk about the response model ? If so indeed i think it's not needed but why would I remove it ?

antarest/study/web/studies_blueprint.py Outdated Show resolved Hide resolved
@MartinBelthle
Copy link
Contributor Author

After discussing with Sylvain, we decided that this endpoint will be strictly used by the front-end so we''l had -ui at the end.
If someone wants to use it via API we'll design a new endpoint with the structure Sylvain proposed as it's indeed better for an API user

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Dec 2, 2024
@hdinia hdinia requested a review from skamril December 2, 2024 16:06
@hdinia hdinia self-assigned this Dec 2, 2024
@hdinia
Copy link
Member

hdinia commented Dec 2, 2024

After discussing with Sylvain, we decided that this endpoint will be strictly used by the front-end so we''l had -ui at the end. If someone wants to use it via API we'll design a new endpoint with the structure Sylvain proposed as it's indeed better for an API user

I get the intent of marking UI-only endpoints 👍🏼 , but -ui as a suffix breaks REST conventions where URLs represent resources, not their usage. A prefix like /v1/ui/ would be preferable for UI related routes

@hdinia hdinia force-pushed the feat/add-digest-endpoint branch from d5de960 to 75aacaa Compare December 3, 2024 08:56
@hdinia hdinia force-pushed the feat/add-digest-endpoint branch from 75aacaa to dcc8a96 Compare December 3, 2024 09:31
@hdinia hdinia force-pushed the feat/add-digest-endpoint branch from deb0118 to a932a70 Compare January 31, 2025 15:48
@MartinBelthle MartinBelthle force-pushed the feat/add-digest-endpoint branch from a932a70 to deb0118 Compare January 31, 2025 15:55
@hdinia hdinia force-pushed the feat/add-digest-endpoint branch from 78d11a4 to 8a2dcf7 Compare January 31, 2025 17:11
@hdinia hdinia force-pushed the feat/add-digest-endpoint branch from 8a2dcf7 to 0556cd4 Compare February 5, 2025 17:36
@hdinia hdinia force-pushed the feat/add-digest-endpoint branch from 0556cd4 to dd358a2 Compare February 6, 2025 08:56
@hdinia hdinia changed the title feat(api): add digest-specific endpoint feat(ui-api, digest): enhance DigestDialog and digest UI specific endpoint Feb 6, 2025
@hdinia hdinia changed the title feat(ui-api, digest): enhance DigestDialog and digest UI specific endpoint feat(ui-api, digest): enhance DigestDialog and add dedicated endpoint for digest UI Feb 6, 2025
@MartinBelthle MartinBelthle merged commit 75f761b into dev Feb 10, 2025
15 of 16 checks passed
@MartinBelthle MartinBelthle deleted the feat/add-digest-endpoint branch February 10, 2025 08:45
@makdeuneuv makdeuneuv added this to the v3.0.0 milestone Feb 12, 2025
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.

6 participants