Skip to content

pagination to support entities in b2b #389

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
Merged
melnikovi merged 6 commits into from
Sep 28, 2020
Merged

pagination to support entities in b2b #389

melnikovi merged 6 commits into from
Sep 28, 2020

Conversation

@dani97
Copy link
Contributor

@dani97 dani97 commented Jun 19, 2020 •
edited
Loading

Problem

Current Graphql endpoints like cartItems, address, order items etc are not paginated. In B2B scenarios, these could impact performance because of large number of items will be returned in collection.

Solution

Adding pagination for entities.
Cart Items,
Order Items,
Customer Addresses

Requested Reviewers

@nrkapoor
@paliarush
@DrewML

@dani97
Copy link
Contributor Author

dani97 commented Jun 19, 2020

@nrkapoor @paliarush opened PR for pagination in collection that can impact b2b performance.

@dani97
Copy link
Contributor Author

dani97 commented Jun 19, 2020

@paliarush please advise for backward compatibility, will convert it into PR once we resolve it.

@DrewML DrewML mentioned this pull request Jun 23, 2020
Merged
@dani97 dani97 force-pushed the b2b-cart-address-order-req-list-pagination branch from fd7d860 to eb5b7b3 Compare June 25, 2020 14:34
@DrewML
Copy link
Contributor

DrewML commented Jun 25, 2020 •
edited
Loading

@dani97 Hey! Just an FYI, @paliarush is out of the office right now, and I'm a bit backed up on reviews of other things. We'll try to get to this one, but expect some delays.

This is a very important change though, and I'm glad you're starting the discussion!

@dani97
Copy link
Contributor Author

dani97 commented Jun 25, 2020

@DrewML thanks. I have split this pr to #391. If req list pagination get merged first, it will help us developing requisition list implementation.

@paliarush
Copy link
Contributor

paliarush commented Jul 27, 2020

@dani97 please move the PR from Draft status to Open if still planning to contribute the changes.

@dani97 dani97 force-pushed the b2b-cart-address-order-req-list-pagination branch from eb5b7b3 to 427ef5b Compare July 28, 2020 05:09
@dani97 dani97 marked this pull request as ready for review July 28, 2020 05:10
paliarush
paliarush reviewed Aug 3, 2020
View reviewed changes
design-documents/graph-ql/coverage/PaginationChangesForEntities.graphqls Outdated
@@ -0,0 +1,25 @@
type Cart {
items(
Copy link
Contributor

@paliarush paliarush Aug 3, 2020

Choose a reason for hiding this comment

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

If we need field type change, a new field with version suffix should be introduced, items_v2 in this case.

Copy link
Contributor

@paliarush paliarush Aug 3, 2020

Choose a reason for hiding this comment

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

The same is related to other entities in this list.

Copy link
Contributor Author

@dani97 dani97 Aug 5, 2020

Choose a reason for hiding this comment

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

should we deprecate the field?

Copy link
Member

@melnikovi melnikovi Sep 8, 2020

Choose a reason for hiding this comment

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

Yes, let's deprecate the old fields.

design-documents/graph-ql/coverage/PaginationChangesForEntities.graphqls Outdated
@@ -0,0 +1,25 @@
type Cart {
Copy link
Contributor

@paliarush paliarush Aug 3, 2020

Choose a reason for hiding this comment

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

When possible, please locate the original proposal with the schema and modify it there (like it is done for customer-orders). Thanks

@dani97 dani97 force-pushed the b2b-cart-address-order-req-list-pagination branch from 427ef5b to 092cf63 Compare August 5, 2020 05:37
@dani97 dani97 requested a review from paliarush August 5, 2020 15:17
@dani97
Copy link
Contributor Author

dani97 commented Aug 5, 2020

@paliarush made the changes 👍

@dani97
Copy link
Contributor Author

dani97 commented Sep 1, 2020

@melnikovi

melnikovi
melnikovi requested changes Sep 8, 2020
View reviewed changes
@melnikovi
Copy link
Member

melnikovi commented Sep 8, 2020

Please resolve the merge conflicts.

@melnikovi melnikovi added the needs update Author should update the proposal based on the feedback label Sep 11, 2020
@dani97 dani97 force-pushed the b2b-cart-address-order-req-list-pagination branch from 1054404 to b28e922 Compare September 12, 2020 14:21
dani97
dani97 commented Sep 12, 2020
View reviewed changes
dani97
dani97 commented Sep 12, 2020
View reviewed changes
@dani97
Copy link
Contributor Author

dani97 commented Sep 12, 2020

@melnikovi resolved and added pagination to wishlist

@melnikovi melnikovi self-assigned this Sep 28, 2020
@melnikovi melnikovi self-requested a review September 28, 2020 19:16
@melnikovi melnikovi merged commit e1be8f3 into magento:master Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@melnikovi melnikovi melnikovi approved these changes

@paliarush paliarush Awaiting requested review from paliarush

Assignees

@melnikovi melnikovi

Labels

GraphQL needs update Author should update the proposal based on the feedback Partner: Zilker Technology partners-contribution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dani97 @DrewML @paliarush @melnikovi @magento-engcom-team