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(link): add update endpoint for link #2175

Merged
merged 178 commits into from
Nov 29, 2024

Conversation

TheoPascoli
Copy link
Contributor

Add an endpoint to update link

antarest/study/web/study_data_blueprint.py Outdated Show resolved Hide resolved
@@ -198,6 +197,24 @@ def create_link(
params = RequestParameters(user=current_user)
return study_service.create_link(uuid, link_creation_info, params)

@bp.put(
"/studies/{uuid}/links",
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. We should probably discuss what's a link id then. Is it area1%area2 or area1/area2 or another id ?

antarest/study/service.py Show resolved Hide resolved
antarest/study/business/model/link_model.py Outdated Show resolved Hide resolved
antarest/study/business/link_management.py Outdated Show resolved Hide resolved
tests/integration/study_data_blueprint/test_link.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MartinBelthle MartinBelthle left a comment

Choose a reason for hiding this comment

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

Really minor changes, otherwise looks good to me. With the matrix tests we'll be good to go

antarest/study/business/link_management.py Outdated Show resolved Hide resolved
antarest/study/business/link_management.py Outdated Show resolved Hide resolved
antarest/study/business/link_management.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MartinBelthle MartinBelthle left a comment

Choose a reason for hiding this comment

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

Still got little issues

MartinBelthle
MartinBelthle previously approved these changes Nov 27, 2024
Added new linkDtoForUpdate object to hide area1 and area2 from the client
Copy link
Contributor

@MartinBelthle MartinBelthle left a comment

Choose a reason for hiding this comment

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

2 minor comments that don't really need to be adress and 1 question

@@ -78,6 +86,11 @@ def to_internal(self, version: StudyVersion) -> "LinkInternal":
return LinkInternal(**data)


class LinkDtoForUpdate(LinkDTO):
area1: SkipJsonSchema[str] = Field("a", exclude=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird way to do it. Why did you introduce fields a and b ?

Copy link
Contributor

Choose a reason for hiding this comment

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

After testing this allows user to specfiy area1 and area2 inside the body and they we'll be ignored. I don't think we want that. I'd prefer you introduce a class that just has the base attributes and make LinkDTO inherit from this :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it

return self


class LinkDTO(Area):
model_config = ConfigDict(alias_generator=to_camel_case, populate_by_name=True, extra="forbid")
model_config = ConfigDict(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be in one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

@@ -78,6 +86,11 @@ def to_internal(self, version: StudyVersion) -> "LinkInternal":
return LinkInternal(**data)


class LinkDtoForUpdate(LinkDTO):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be DTO instead of Dto to fit with existing class

@TheoPascoli TheoPascoli changed the title feat(link): add endpoint link update feat(link): add update endpoint for link Nov 29, 2024
@TheoPascoli TheoPascoli merged commit aabf89f into dev Nov 29, 2024
10 of 12 checks passed
@TheoPascoli TheoPascoli deleted the feature/add-endpoint-link-update branch November 29, 2024 08:45
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.

5 participants