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

Bulk delete #611

Merged
Merged

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Feb 20, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #X, partially #Y, mentioned in #Z
License MIT

@loic425 loic425 force-pushed the feature/bulk-delete branch from 5473e6f to e97d1a3 Compare February 20, 2023 14:20
@loic425 loic425 requested a review from a team as a code owner February 20, 2023 14:20
@loic425 loic425 force-pushed the feature/bulk-delete branch from f9dc583 to 8ffcbad Compare February 23, 2023 11:07
@loic425 loic425 force-pushed the feature/bulk-delete branch 4 times, most recently from de91364 to fbbc9d6 Compare March 10, 2023 08:29
#[Assert\NotBlank]
#[Assert\Email]
#[ORM\Column(name: 'name', nullable: false)]
public ?string $email = null,
) {
}

public function getId(): ?Uuid
public function getId(): ?int
Copy link
Member Author

Choose a reason for hiding this comment

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

I've simplified this cause I had some troubles. But it works on a test project with an uuid on a mysql database.

Copy link
Member

Choose a reason for hiding this comment

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

What were the troubles?

Copy link
Member Author

@loic425 loic425 Mar 13, 2023

Choose a reason for hiding this comment

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

The findById($ids) did not work with SQLite, but I have to check it that again later.
But if there is a bug with that on SQLite, we already have a bug on current implementation.

Comment on lines +145 to +148
$this->client->request('GET', '/admin/subscriptions');
$this->client->submitForm('Bulk delete');

$this->assertResponseRedirects(null, expectedCode: Response::HTTP_FOUND);
Copy link
Member

Choose a reason for hiding this comment

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

Is this test correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, It checks that the redirect is ok. (it's a code from @Zales0123 on ScienceBookUiTest)

@loic425 loic425 force-pushed the feature/bulk-delete branch from 219a7f5 to 735cee2 Compare March 17, 2023 11:24
@lchrusciel lchrusciel added Feature New feature proposals. DX Issues and PRs aimed at improving Developer eXperience. labels Mar 17, 2023
@lchrusciel lchrusciel merged commit 66fa523 into Sylius:poc-new-resource-metadata Mar 17, 2023
@loic425 loic425 deleted the feature/bulk-delete branch March 17, 2023 14:44
Zales0123 added a commit that referenced this pull request Sep 9, 2023
This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no <!-- don't forget to update the UPGRADE-*.md file -->
| Related tickets | fixes #494 
| License         | MIT

Not sure if this is right way to fix the CI

When installing doctrine/collections ^2.0 doctrine/persistence ^3.0 and PHP 8.1 should be used.
doctrine/collections ^2.0 should not be installed on PHP 8.0

What to do with the psalm errors?

Commits
-------

4be530e Allow doctrine/collections ^2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues and PRs aimed at improving Developer eXperience. Feature New feature proposals.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants