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

[Addressing] Prevent deleting zones and provinces that are zone members #14041

Conversation

coldic3
Copy link
Contributor

@coldic3 coldic3 commented Jun 1, 2022

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

It is possible to delete the resource (zone or province) that is already in use as a zone member. When we delete such a resource, we cannot access the zone edit screen (and probably even more) because we try to load an unexisting resource 💥

Sample error:

Unable to transform data for property path "code": Object "Sylius\Component\Addressing\Model\Zone" with identifier "code"="LETS_DELETE_ME" does not exist.

@coldic3 coldic3 requested a review from a team as a code owner June 1, 2022 14:38
@coldic3 coldic3 force-pushed the fix/prevent-deleting-zones-and-provinces-that-are-zone-members branch from 409d8b8 to cc2f3d6 Compare June 2, 2022 12:36
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Jun 2, 2022
@coldic3 coldic3 changed the base branch from master to 1.11 June 2, 2022 12:38
@coldic3 coldic3 added the Bug Confirmed bugs or bugfixes. label Jun 2, 2022
@coldic3 coldic3 force-pushed the fix/prevent-deleting-zones-and-provinces-that-are-zone-members branch 3 times, most recently from 607c45e to 610832e Compare June 3, 2022 08:52
@coldic3 coldic3 changed the title [WIP][Addressing] Prevent deleting zones and provinces that are zone members [Addressing] Prevent deleting zones and provinces that are zone members Jun 3, 2022
{
Assert::contains(
$this->responseChecker->getError($this->client->getLastResponse()),
'Cannot delete, the province is in use.'
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message and Error Cannot delete, the province is in use. src/Sylius/Behat/Context/Ui/Admin/ManagingCountriesContext.php:284 shouldn't be more likely the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Error" before the error message is something that exists only in flash messages as a message header.
image

I think it's a good idea to compare just a message in the UI context but I would be for doing it in separate PR as it requires changes in many behat contexts.

@coldic3 coldic3 force-pushed the fix/prevent-deleting-zones-and-provinces-that-are-zone-members branch from 610832e to 199f7d5 Compare June 6, 2022 06:51
@coldic3 coldic3 force-pushed the fix/prevent-deleting-zones-and-provinces-that-are-zone-members branch from 199f7d5 to 1e650c9 Compare June 6, 2022 09:44
@GSadee GSadee merged commit ddeb60f into Sylius:1.11 Jun 6, 2022
@GSadee
Copy link
Member

GSadee commented Jun 6, 2022

Thank you, Kevin! 🥇

@coldic3 coldic3 deleted the fix/prevent-deleting-zones-and-provinces-that-are-zone-members branch June 6, 2022 12:45
GSadee added a commit that referenced this pull request Jun 8, 2022
This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.11          |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no |
| Related tickets | #14041                      |
| License         | MIT                                                          |

Without orphan removal, even if we delete the zone member, the corresponding province/country cannot be removed because orphans still exist in the database.

Commits
-------

47fef6e [Addressing] Enable orphan removal for zone members
GSadee added a commit that referenced this pull request Jun 8, 2022
This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         |  1.11         |
| Bug fix?        | yes (for Symfony 6)                                                  |
| New feature?    | no                                                       |
| BC breaks?      | not yet                                                       |
| Deprecations?   | no |
| Related tickets | partially #13274 related #14041                        |
| License         | MIT                                                          |

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

46cbdef Fix zone member integrity listener
138b136 Keep session for now
561867a Fix Psalm errors
9ab8098 Trying to fix Phpspec on Symfony 4.4
af47339 Fix Phpspec errors again
9ade6f2 Apply suggestions from code review
c165241 Use request stack service everytimes
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. Bug Confirmed bugs or bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants