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

Add composite primary keys tests and use Paginator #1268

Merged
merged 4 commits into from
Jan 18, 2021
Merged

Add composite primary keys tests and use Paginator #1268

merged 4 commits into from
Jan 18, 2021

Conversation

VincentLanglet
Copy link
Member

@VincentLanglet VincentLanglet commented Jan 17, 2021 •
edited
Loading

I added some functional tests for composite primary keys.

https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1268/checks?check_run_id=1717007313 show the failure if I removed the code for composite keys.

Then I added the PR #1217, to check that the code is still working fine.

Changelog

### Changed
- Use Doctrine ORM Paginator to count in Pager.

### Fixed
- Support of composite key for computeNbResult

@VincentLanglet VincentLanglet force-pushed the compositePrimaryKeys branch 3 times, most recently from 1603c5f to 81b5038 Compare January 17, 2021 14:44
@VincentLanglet VincentLanglet requested a review from a team January 17, 2021 15:07
@VincentLanglet VincentLanglet marked this pull request as ready for review January 17, 2021 15:08
@VincentLanglet VincentLanglet changed the title Add composite primary keys tests Add composite primary keys tests and use Paginator Jan 17, 2021
@VincentLanglet VincentLanglet mentioned this pull request Jan 17, 2021
Closed
@franmomu
Copy link
Member

If I had understood correctly #1217 (comment), the problem is when the composite keys are relations, with scalar values should work fine, can you please add a test with relations?

@VincentLanglet VincentLanglet force-pushed the compositePrimaryKeys branch 5 times, most recently from fee066b to af7566c Compare January 17, 2021 21:00
@VincentLanglet
Copy link
Member Author

Done @franmomu :)

phansys
phansys previously approved these changes Jan 18, 2021
View reviewed changes
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

I think these tests are fine.
I just have a concern about using real trademarks in the fixtures. I think we should avoid them if possible.

@VincentLanglet
Copy link
Member Author

I removed the trademarkes @phansys

@OskarStark OskarStark merged commit 3e0d45d into sonata-project:3.x Jan 18, 2021
@kirya-dev kirya-dev mentioned this pull request Jan 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phansys phansys phansys approved these changes

@franmomu franmomu franmomu approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@VincentLanglet @franmomu @phansys @OskarStark