-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
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.
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)
antarest/study/storage/rawstudy/model/filesystem/root/output/simulation/mode/mcall/digest.py
Outdated
Show resolved
Hide resolved
antarest/study/storage/rawstudy/model/filesystem/root/output/simulation/mode/mcall/digest.py
Show resolved
Hide resolved
from antarest.study.storage.rawstudy.model.filesystem.root.output.simulation.mode.mcall.synthesis import OutputSynthesis | ||
|
||
|
||
class DigestMatrixDTO(AntaresBaseModel): |
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.
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.
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.
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, |
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 don't think this is needed ?
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.
You talk about the response model ? If so indeed i think it's not needed but why would I remove it ?
After discussing with Sylvain, we decided that this endpoint will be strictly used by the front-end so we''l had |
I get the intent of marking UI-only endpoints 👍🏼 , but |
d5de960
to
75aacaa
Compare
antarest/study/storage/rawstudy/model/filesystem/root/output/simulation/mode/mcall/digest.py
Outdated
Show resolved
Hide resolved
75aacaa
to
dcc8a96
Compare
webapp/src/components/common/dialogs/DigestDialog/DigestMatrix.tsx
Outdated
Show resolved
Hide resolved
webapp/src/components/common/dialogs/DigestDialog/DigestTabs.tsx
Outdated
Show resolved
Hide resolved
deb0118
to
a932a70
Compare
a932a70
to
deb0118
Compare
78d11a4
to
8a2dcf7
Compare
8a2dcf7
to
0556cd4
Compare
0556cd4
to
dd358a2
Compare
DigestDialog
and digest UI specific endpoint
DigestDialog
and digest UI specific endpointDigestDialog
and add dedicated endpoint for digest UI
Fix [ANT-2449] for the back-end part
Endpoint: GET "/private/studies/{study_id}/outputs/{output_id}/digest-ui"
ResponseModel: