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

Add "enabled" property to API serialization #12781

Merged
merged 2 commits into from Mar 22, 2023

Conversation

Nek-
Copy link
Contributor

@Nek- Nek- commented Jul 5, 2021

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets none
License MIT

This is a suggestion/feature. But it really makes sense to expose if an item is enabled or not in admin serialization, it's actually a piece of super-critical information for admin data results.

Sorry for the noise of fixing also copyright but my brain was not happy it was not heterogeneous. x) It's split into 2 proper commits if you want to review with ease.

Happy coding guys.

@Nek- Nek- requested a review from a team as a code owner July 5, 2021 14:45
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Jul 5, 2021
@Nek- Nek- changed the title Add enabled to API serialization Add "enabled" property to API serialization Jul 5, 2021
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

absolutely useful, but we should add at least contract test for it

@lchrusciel
Copy link
Member

tests to extend can be found here: https://github.com/Sylius/Sylius/blob/1.13/tests/Api/Admin/ProductVariantsTest.php. This flag needs to be set in fixtures and exposed in json samples. Such tests have to be done for each resource that is tackled here. What is more, rebase is needed

@Nek- Nek- force-pushed the feature/add-missing-copyright branch from 39c31f4 to 429c6b0 Compare December 12, 2022 14:52
@Nek-
Copy link
Contributor Author

Nek- commented Dec 12, 2022

@lchrusciel updated 😊

@jakubtobiasz jakubtobiasz changed the base branch from 1.12 to 1.13 February 3, 2023 11:54
@jakubtobiasz
Copy link
Member

Hi @Nek-!
Thanks for your contribution. As it is a feature, I've bumped the target branch to 1.13. Unfortunately, some conflicts appeared. Can you try to resolve them? In exchange, I can promise I'll merge it before next version 😂.

@Nek- Nek- force-pushed the feature/add-missing-copyright branch from 429c6b0 to 729a617 Compare February 5, 2023 14:55
@Nek-
Copy link
Contributor Author

Nek- commented Feb 5, 2023

@jakubtobiasz done!

@jakubtobiasz
Copy link
Member

Hi @Nek-!
Thanks for rebase. I see there are failing tests, can you check it 👀?

@Nek- Nek- force-pushed the feature/add-missing-copyright branch 2 times, most recently from b324640 to a9e20ef Compare March 17, 2023 17:35
@Nek-
Copy link
Contributor Author

Nek- commented Mar 17, 2023

@jakubtobiasz Sorry for the delay, it should be fine now.

Knowing if an item is enable or not is a critical information while
querying the API in admin mode. This feature ("enabled") has been
introduced in Sylius 1.8 and is exposed thanks to this commit.
@Nek- Nek- force-pushed the feature/add-missing-copyright branch from a9e20ef to 53621c9 Compare March 19, 2023 10:02
@NoResponseMate NoResponseMate merged commit a0e9a99 into Sylius:1.13 Mar 22, 2023
@NoResponseMate
Copy link
Contributor

Thank you, @Nek-!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants