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(api): add digest-specific endpoint #2240

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from

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
return Array.isArray(columns[0]);
};

const isColumns = (columns: string[] | string[][]): columns is string[] => {
Copy link
Member

@skamril skamril Dec 12, 2024

Choose a reason for hiding this comment

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

Opinion: Not necessary to create this one, it's more clear to use isGroupedColumns every time for me

const tabItems = [
{
label: "Area",
content: matrices.area && <DigestMatrix matrix={matrices.area} />,
Copy link
Member

Choose a reason for hiding this comment

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

It's not better to hide tabs not used?

const tabItems = [
  matrices.area && { label: "Area", ... },
  ....
].filter(Boolean)

////////////////////////////////////////////////////////////////

return (
<Box sx={{ height: "100%", display: "flex", flexDirection: "column" }}>
Copy link
Member

Choose a reason for hiding this comment

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

Tip: in sx prop height: "100%" is same as height: 1

response={digestRes}
ifPending={() => <Skeleton sx={{ height: 1, transform: "none" }} />}
ifFulfilled={(matrices) =>
matrices.data && <DigestTabs matrices={matrices.data} />
Copy link
Member

Choose a reason for hiding this comment

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

No message for empty matrice?

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.

4 participants