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

comprehensive param support for ra-data-graphql-simple adapter GET_MANY_REFERENCE requests #3759

Merged
merged 6 commits into from Nov 21, 2019

Conversation

maxschridde1494
Copy link
Contributor

Support comprehensive pagination, sort and filter params for GET_MANY_REFERENCE requests. currently there is a gap using the ra-data-graphql-simple adapter when trying to use filter / sort / pagination props on the ReferenceManyField component.

@maxschridde1494
Copy link
Contributor Author

The reason for this pull request is quite simple. I have a react-admin app running against a graphql api and decided to use the ra-data-graphql-simple adapter. Everything has worked perfectly except for the ReferenceManyField component. Currently, the ra-data-graphql-simple adapter does not support filtering, pagination and sorting params. i.e. code such as
<ReferenceManyField filter={{ type: 'CorporateDocument' }} reference="KycDocument" target="merchant.id" >
would issue a request like
{ operationName: "allKycDocuments", query: "query allKycDocuments($filter: KycDocumentFilter) ... , variables: { filter: {merchantId: "0af834d9-aa7a-423c-be16-33ea6a724006"} } }
when it should be
{ operationName: "allKycDocuments", query: "query allKycDocuments($filter: KycDocumentFilter) ... , variables: { filter: { merchantId: "0af834d9-aa7a-423c-be16-33ea6a724006", type: "IdentityDocument" } } }
I noticed discussion on this topic in closed issue #2571, specifically #2571 (comment). I piggy backed off @wmwart suggestion to use buildGetListVariables in the GET_MANY_REFERENCE case statement.

@maxschridde1494
Copy link
Contributor Author

Also, due to the fact that I inserted an if statement in the buildGetListVariables for a simple check, make prettier made the diff of this function look way more complicated than it actually is. The changes were simple. I changed line buildVariables.js#103 to
let variables = { filter: {} }; if (params.filter) { variables.filter = Object.keys(params.filter).reduce((acc, key) => { ...
(i.e. only running the existing reducer that creates the filter object if params.filter is not null)

@wmwart
Copy link

wmwart commented Oct 2, 2019

@maxschridde1494, great news)
There is still a problem that I have not mentioned before. When using ReferenceManyField, for example:

<ReferenceManyField ... pagination={<Pagination />} >
    <Datagrid rowClick="show">
        ...
    </Datagrid>
</ReferenceManyField>

, and go to the page !== 1 , then click on row and return back to Datagrid. The page is reset to 1 :(
Very annoying. Is there such a problem with your corrections?

@fzaninotto
Copy link
Member

As this is a new feature, please PR against the next branch rather than master

Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Awesome :) Thanks!

@maxschridde1494 maxschridde1494 changed the base branch from master to next October 2, 2019 15:45
@maxschridde1494
Copy link
Contributor Author

@fzaninotto updated to PR against next.

@wmwart re pagination, my changes do not fix that issue. I believe that is a different bug that has to do with the ReferenceManyField component itself, and not the dataProvider adapter.

@Kmaschta
Copy link
Contributor

Kmaschta commented Oct 2, 2019

You'll have to keep only your commit on that branch.
To do so, you can run :

git checkout next && git pull origin next
git checkout patch-1
git rebase -i next ## In the prompt, only keep your commit and save
git push -f

maxschridde1494 and others added 5 commits October 2, 2019 09:53
support comprehensive pagination, sort and filter params for GET_MANY_REFERENCE requests. currently there is a gap using the ra-data-graphql-simple adapter when trying to use filter / sort / pagination props on the ReferenceManyField component.
@maxschridde1494
Copy link
Contributor Author

@Kmaschta done.

@christiaanwesterbeek
Copy link
Contributor

As this is a new feature, please PR against the next branch rather than master

This is unfortunate since it won't get into 2.x or am I wrong about that?

Related bug report: #3397

@maxschridde1494
Copy link
Contributor Author

Any update on when this might get merged?

@fzaninotto fzaninotto added this to the 3.1.0 milestone Oct 15, 2019
@fzaninotto
Copy link
Member

3.0 is feature freeze - we're focusing on fixing bugs. I added that PR to the 3.1 release, scheduled in November.

By the way, this PR needs a rebase.

@maxschridde1494
Copy link
Contributor Author

maxschridde1494 commented Oct 15, 2019

Sounds good.

Re:

By the way, this PR needs a rebase.

Will this be done on your end or is it something that I need to complete. I already completed the rebase @Kmaschta directed:

You'll have to keep only your commit on that branch.
To do so, you can run :

git checkout next && git pull origin next
git checkout patch-1
git rebase -i next ## In the prompt, only keep your commit and save
git push -f

@fzaninotto
Copy link
Member

It is something you'll have to do, maybe using the "resolve conflicts" button lower in the page.

@maxschridde1494
Copy link
Contributor Author

When is the scheduled release for v3.1?

@fzaninotto fzaninotto requested a review from djhi November 21, 2019 14:46
@djhi djhi merged commit 6194bbb into marmelab:next Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants