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.

Sign up for GitHub

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

Jump to bottom

feat: add holistic durability in exploratory mode #721

Merged
merged 23 commits into from
Sep 10, 2024
Merged

feat: add holistic durability in exploratory mode #721

merged 23 commits into from
Sep 10, 2024

Conversation

vjousse
Copy link
Collaborator

@vjousse vjousse commented Sep 3, 2024 •
edited
Loading

🔧 Problem

Add physical durability in exploratory mode.

Notion card

📖 TODO

  • Add physical durability concept to elm data types
  • Add physical durability to the API

🏝️ How to test

Play with the new durability slider and check that everything is ok.
Checking the behavior of the API may be a good idea too.

@vjousse vjousse requested a review from n1k0 September 5, 2024 09:23
n1k0
n1k0 reviewed Sep 5, 2024
View reviewed changes
Copy link
Member

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

There's a disturbing behavior when setting the physical durability then update the non-physical one. Is it expected? Is it wanted?

REC-20240905152253.mp4

src/Data/Textile/Query.elm Outdated Show resolved Hide resolved
src/Data/Textile/Query.elm Outdated Show resolved Hide resolved
src/Data/Unit.elm Outdated Show resolved Hide resolved
src/Page/Textile.elm Outdated Show resolved Hide resolved
src/Server/Query.elm Outdated Show resolved Hide resolved
src/Views/RangeSlider.elm Outdated Show resolved Hide resolved
@vjousse vjousse marked this pull request as ready for review September 9, 2024 14:32
@vjousse vjousse requested a review from n1k0 September 9, 2024 14:35
n1k0
n1k0 reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

A few more cosmetic suggestions

src/Page/Textile.elm Outdated Show resolved Hide resolved
src/Page/Textile.elm Outdated Show resolved Hide resolved
n1k0
n1k0 reviewed Sep 10, 2024
View reviewed changes
src/Page/Api.elm Show resolved Hide resolved
n1k0
n1k0 reviewed Sep 10, 2024
View reviewed changes
src/Server/Query.elm
Comment on lines +275 to +281
Err
("La durabilité doit être comprise entre "
++ String.fromFloat minFloatDurability
++ " et "
++ String.fromFloat maxFloatDurability
++ "."
)
Copy link
Member

@n1k0 n1k0 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For some reason this doesn't seem to work:
image

Also, we'll probably want a unit test in tests/RouteTest.elm to check that validation works

EDIT: see this comment

Copy link
Member

Choose a reason for hiding this comment

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

It also appears that the new field is not documented for the POST endpoint in the OpenAPI schema
image

Copy link
Member

Choose a reason for hiding this comment

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

I've added the param docs for POST in da0fff1

Copy link
Member

Choose a reason for hiding this comment

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

For the lack of validation in POST, this is a MUCH broader issue I've documented in this comment

@n1k0
Copy link
Member

n1k0 commented Sep 10, 2024 •
edited
Loading

@vjousse I've added a failing test in 95bc36c highlighting we don't validate JSON queries passed to the POST API endpoints. This is an issue much broader than the scope of this patch and I don't know if this should be addressed here or in a next dedicated PR (I'm thinking the latter, but I'd love hearing your thoughts)

Edit: it's been decided to address the POST validation stuff in a dedicated PR.

vjousse and others added 5 commits September 10, 2024 11:19
@vjousse
refactor: sort all the things
fdd730a
@vjousse
Merge branch 'feat/add-physical-durability' of github.com:MTES-MCT/ec…
86d550b 
…obalyse into feat/add-physical-durability
@n1k0
Revert "test: add a failing test highlighting no JSON query validation."
09bc57e 
This reverts commit 95bc36c.
@vjousse @n1k0
chore: styling
d3bf24b 
Co-authored-by: Nicolas Perriault <nicolas@perriault.net>
@vjousse @n1k0
chore: styling
fef1dec 
Co-authored-by: Nicolas Perriault <nicolas@perriault.net>
@vjousse vjousse requested a review from n1k0 September 10, 2024 13:18
n1k0
n1k0 approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

LGTM, ship it!

@vjousse vjousse merged commit 774faf3 into master Sep 10, 2024
6 checks passed
@vjousse vjousse deleted the feat/add-physical-durability branch September 10, 2024 13:34
vjousse added a commit that referenced this pull request Sep 10, 2024
@vjousse @n1k0
feat: add holistic durability in exploratory mode (#721)
5c51ba7 
## 🔧 Problem

Add physical durability in exploratory mode.

[Notion
card](https://www.notion.so/Introduire-la-durabilit-physique-en-mode-exploratoire-7466a0fd69f140f984cf85b272bbe5ae)

## 📖  TODO

- [x] Add physical durability concept to elm data types
- [x] Add physical durability to the API

## 🏝️ How to test

Play with the new durability slider and check that everything is ok.
Checking the behavior of the API may be a good idea too.

---------

Co-authored-by: Nicolas Perriault <nicolas@perriault.net>
@github-actions github-actions bot mentioned this pull request Sep 10, 2024
Merged
vjousse pushed a commit that referenced this pull request Sep 12, 2024
@github-actions
chore(master): release 2.2.0 (#717)
61340c5 
[2.2.0](v2.1.1...v2.2.0)
(2024-09-12)


### Features

* add app version to openapi docs in the root endpoint.
([#726](#726))
([5959c34](5959c34))
* add holistic durability in exploratory mode
([#721](#721))
([774faf3](774faf3))
* render app version details in the changelog.
([#725](#725))
([8f6ea50](8f6ea50))


### Bug Fixes

* accept custom making complexity for upcycled garments.
([#723](#723))
([8f61547](8f61547))
* **ci:** check for ecobalyse-private when extracting the branch name
([#733](#733))
([23ae8a5](23ae8a5))
* don't add disabled step impacts to lifecycle totals.
([#719](#719))
([b6a7e1c](b6a7e1c))
* ensure express app is properly monitored by Sentry.
([#729](#729))
([84a39aa](84a39aa))
* make scalingo not segfaulting.
([#728](#728))
([1de5140](1de5140))
* use fabric processes to compute fabric waste
([#712](#712))
([1cce55b](1cce55b))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Oct 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n1k0 n1k0 n1k0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vjousse @n1k0