-
Notifications
You must be signed in to change notification settings - Fork 45
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
front : fix step update erasing parts of the study in studies page #10382
base: dev
Are you sure you want to change the base?
Conversation
…partial erasure Signed-off-by: Alice Khoudli <alice.khoudli@polytechnique.org>
Signed-off-by: Alice Khoudli <alice.khoudli@polytechnique.org>
… as finished in study page Signed-off-by: Alice Khoudli <alice.khoudli@polytechnique.org>
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10382 +/- ##
==========================================
+ Coverage 81.65% 81.77% +0.12%
==========================================
Files 1066 1073 +7
Lines 105692 106531 +839
Branches 727 727
==========================================
+ Hits 86302 87119 +817
- Misses 19349 19373 +24
+ Partials 41 39 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -4,10 +4,10 @@ | |||
"createScenario": "Create a scenario", | |||
"dates": { | |||
"creation": "Creation", | |||
"estimatedend": "Expected ending", |
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.
estimatedEnd for readability ? 😄
or maybe it should be expectedEnd ?
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.
Good catch, lets go with expectedEnd ^^
…ject using serde with double option Signed-off-by: Alice Khoudli <alice.khoudli@polytechnique.org>
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.
Great improvement for editoast
form's. We should actually bring that to other forms now that we know 😄.
I actually tested a PUT
on /studies
with {"description":"something"}
, then with {"study_type":"something"}
(checking that description
has not been erased in the process) and it works well. Then finally with {"description":null}
which works also as expected.
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.
LGTM, thanks!
That's actually quite a lot of them 😨 I came to really dislike the |
Close #6455
I believe the issue came from the fact editoast does not properly handles a partial studyForm, as the attributes are nullable and editoast does not seem to distinguish between an undefined attribute and a null one. The study changeset is indeed option<option>, but the studyForm is simply option. I am unsure if we can handle this better, considering the limitations of open api and type conversions between js and rust.
In the meantime, we now send the full study everytime.
Also fix a couple inconsistent translations in study dates
Edit : patches now properly handled in editoast thanks to serde with double option, we only send partial study patches again