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

fixes #24050 - graphql: add connections with totalCount #5733

Merged

Conversation

timogoebel
Copy link
Member

@timogoebel timogoebel commented Jun 23, 2018

This is part four of the graphql series.
This adds support for connections (associations) to the models. It adds a basic host type that just has name, id and model attributes.


Other parts:
Part 1 - JWT Auth: #5596
Part 2 - Graphql Scaffolding: #5680
Part 3 - Relay Global ID #5681
Part 4 - Connections with totalCount #5733
Part 5 - Basic mutations for model #5736

@theforeman-bot
Copy link
Member

Issues: #24050

Copy link

@dariuszb-iRonin dariuszb-iRonin left a comment

Choose a reason for hiding this comment

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

I've got one suggestion and I think we can try to DRY connections definitions.

app/graphql/connections/hosts_connection.rb Outdated Show resolved Hide resolved
Copy link

@dariuszb-iRonin dariuszb-iRonin left a comment

Choose a reason for hiding this comment

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

I checked my suggestion from previous review about changes for connections, but I wasn't able to prepare workable solution.
I added one new comment and another proposition(which I checked locally) for drying up code of connections.

app/graphql/types/host.rb Outdated Show resolved Hide resolved
app/graphql/connections/hosts_connection.rb Outdated Show resolved Hide resolved
@timogoebel timogoebel force-pushed the 24050-graphql-connection-total-count branch from d9a02cc to af3822a Compare June 28, 2018 09:57
@timogoebel
Copy link
Member Author

Rebased and applied @dariuszb-iRonin's suggestion.

@timogoebel
Copy link
Member Author

Rebased.

@timogoebel timogoebel force-pushed the 24050-graphql-connection-total-count branch from a9ed394 to 5ba58b0 Compare March 5, 2019 21:07
@timogoebel timogoebel requested a review from xprazak2 March 7, 2019 09:26
@timogoebel timogoebel force-pushed the 24050-graphql-connection-total-count branch from 5ba58b0 to e15d4c4 Compare March 9, 2019 21:16
@xprazak2
Copy link
Contributor

xprazak2 commented Mar 12, 2019

Works well, but needs a rebase.

@timogoebel
Copy link
Member Author

Rebased.

@timogoebel
Copy link
Member Author

@xprazak2: Do you have a chance to take a look at this PR this morning? If we merge any other graphql PRs before this, we'll have to rebase again.

kamils-iRonin
kamils-iRonin previously approved these changes Mar 13, 2019
@timogoebel
Copy link
Member Author

Rebased.

@xprazak2
Copy link
Contributor

Aaaah, I did not manage to hit the button in time. I'll merge if you rebase, I promise 🙂

@timogoebel timogoebel force-pushed the 24050-graphql-connection-total-count branch from 5cee910 to 104e812 Compare March 15, 2019 10:18
@timogoebel
Copy link
Member Author

@xprazak2: You'll get another chance. Rebased (well, actually @kamils-iRonin did it).

Copy link
Contributor

@xprazak2 xprazak2 left a comment

Choose a reason for hiding this comment

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

LocationJSTest failure unrelated, thanks for your patience!

@xprazak2 xprazak2 merged commit 2a9dd58 into theforeman:develop Mar 15, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants