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

Populating nested mutations with other relationships for @update #549

Merged
merged 25 commits into from
Mar 19, 2019

Conversation

liepaja
Copy link
Contributor

@liepaja liepaja commented Jan 5, 2019

  • Added or updated tests
  • Added Docs for all relevant versions

Related Issue

Resolves #396
Resolves #583
Resolves #586

Related to #332

Changes

Adding other relationship capabilities to nested mutations when using @update.

| T indicates presence of a test

------------------------------- One to one/many

  • BelongsTo

  • create T

  • update T

  • delete T

  • connect T

  • disconnect T

  • HasOne

  • create T

  • update T

  • delete T

  • HasMany

  • create T

  • update T

  • delete T

------------------------------- Many to many

  • BelongsToMany
  • create T
  • update T
  • delete T
  • connect T
  • disconnect T
  • sync T

------------------------------- Morph one to one/many

  • MorphOne

  • create T

  • update T

  • delete T

  • connect

  • MorphMany

  • create T

  • update T

  • delete T

  • connect

  • MorphTo

  • no args

@spawnia
Copy link
Collaborator

spawnia commented Jan 6, 2019

Hey, can you please check the changes that are up and coming in #518 ?

Would be nice if you can base your WIP on that, since otherwise it will require another rework afterwards. Thanks

@liepaja
Copy link
Contributor Author

liepaja commented Jan 6, 2019

@spawnia it look like a huge PR, can you point to exactly what you mean? Do you mean the return types?

@spawnia
Copy link
Collaborator

spawnia commented Jan 6, 2019

Take a look at the MutationExecutor and the relevant tests for it.

@spawnia
Copy link
Collaborator

spawnia commented Jan 7, 2019

Hey, i noticed i did not have the changes that i referred to pushed. Sorry for the confusion, you can take a look now.

@liepaja
Copy link
Contributor Author

liepaja commented Jan 8, 2019

Ok, so basically the style

@spawnia
Copy link
Collaborator

spawnia commented Jan 18, 2019

@liepaja can you give me a quick update on your progress with this? A use-case has popped up in our project that requires nested BelongsToMany updates.

@spawnia spawnia added the enhancement A feature or improvement label Jan 18, 2019
@liepaja
Copy link
Contributor Author

liepaja commented Jan 20, 2019

@spawnia sorry for late reply. I became quite busy recently. I plan to finish this PR by next weekends.

# Conflicts:
#	tests/Integration/Schema/Directives/Fields/UpdateDirectiveTests/CoreTest.php
@spawnia
Copy link
Collaborator

spawnia commented Jan 21, 2019

As this has pretty high priority for us, i started working on your branch. Just merged in master, will start adding tests and implementations soon.

@lukadriel7
Copy link

Hello, thank you for the great work. How is it going?

@liepaja
Copy link
Contributor Author

liepaja commented Feb 8, 2019

Ok, i am back on track, will try to finish it today.

@liepaja
Copy link
Contributor Author

liepaja commented Feb 11, 2019

Core functionalities are added, will add tests and it can be merged

@TheJoeSchr
Copy link

that's really exciting to read, thanks!

@lukadriel7
Copy link

頑張れー

@liepaja
Copy link
Contributor Author

liepaja commented Feb 28, 2019

@JoeSchr lets leave it for another pull request, i'll try to make tests today or tomorrow so this could be finally pushed.

@spawnia
Copy link
Collaborator

spawnia commented Feb 28, 2019

@liepaja great progress on this, keep it up :)

@TheJoeSchr
Copy link

@JoeSchr lets leave it for another pull request, i'll try to make tests today or tomorrow so this could be finally pushed.

Hi @liepaja, great work so far, but I actually think that at least detach is quite essential to have, otherwise you could never uncouple a many2many relationship.
But you will probably see that once you write the tests for it, I also needed to use it first to get that something is missing.

@spawnia spawnia changed the title [WIP][Feat] Populating nested mutations with other relationships for @update [WIP] Populating nested mutations with other relationships for @update Mar 6, 2019
@spawnia
Copy link
Collaborator

spawnia commented Mar 13, 2019

Just a quick headsup @liepaja we are probably finishing this up by tomorrow hopefully in the next week 😓 .

@spawnia spawnia changed the title [WIP] Populating nested mutations with other relationships for @update Populating nested mutations with other relationships for @update Mar 19, 2019
@spawnia spawnia requested a review from chrissm79 March 19, 2019 09:51
@spawnia
Copy link
Collaborator

spawnia commented Mar 19, 2019

@liepaja @chrissm79 this should be ready, can you give it a quick review?

@chrissm79
Copy link
Contributor

Fantastic work on this one @liepaja @spawnia, glad to get this one merged in!!

@chrissm79 chrissm79 merged commit dff751c into nuwave:master Mar 19, 2019
@liepaja
Copy link
Contributor Author

liepaja commented Mar 20, 2019

@liepaja @chrissm79 this should be ready, can you give it a quick review?

Hey guys, sorry that couldn't properly finish PR. Got a tone of projects started, even a new youtube series :D.

@TheJoeSchr
Copy link

Hi,
I just tried to implement this new features for real in my project. I encounter a roadblock with a belongsToMany relationship, which has a pivot table (originally I'm using withPivot(...)->as('betterName'), but for the sake of debugging I left that one out):

class Patient extends Model
{
    // many to many RELATIONSHIPS
    public function medications(): BelongsToMany
    {
        return $this->belongsToMany(Medication::class, 'patients__medications', 'patient_id', 'medication_id')->withPivot('dose_mg', 'is_rescue_medication'); //->as("details");
    }
}
#schema.graphql excerpts
# Mutations {}
updateMedication(input: MedicationInput): Medication! @update(flatten: true)

# Types
# -- Medication
input MedicationInput {
    id: ID
    name: String
    pivot: MedicationDetailsInput
}

input MedicationDetailsInput {
    patient_id: ID
    medication_id: ID
    dose_mg: Float
    is_rescue_medication: Boolean
}
input MedicationRelation {
  update: [MedicationInput]
  connect: [MedicationDetailsInput]
  disconnect: [ID]
  create: [MedicationInput]
  delete: [MedicationInput]
  sync: [MedicationDetailsInput]
}

the error message I get is:

this seems to be the line where things are going wrong, since pivot property (and other name defined with as()) is not treated as special case:
image

sadly I don't know where to go from here, how I could resolve if there is a pivot defined, before passing it on into $remaining. and further how to than flatten/save it so laravel knows what to do with the pivot property.

but let me know if I can help you somehow to fix this.

@spawnia
Copy link
Collaborator

spawnia commented Mar 27, 2019

@JoeSchr there is nothing in Lighthouse right now that allows saving data to a pivot table directly through a nested BelongsToMany mutation.

You can treat your BelongsTo relation simply as two BelongsTo relationships to work around this.

If you want two allow changing pivot data as part of nested mutations, feel free to open up a new feature proposal/ PR. I am open for working that in, but the details for how the API might look are unclear to me.

@TheJoeSchr
Copy link

TheJoeSchr commented Mar 27, 2019

@spawnia Thanks, I worked some more on this and I think I'm almost there...

I had two problems, one I fixed, but in a hacky way ($accessor which has the name of the pivot property, is protected) and the other (how to save the pivot property with the model relation we already have) I'm still working on.

@TheJoeSchr
Copy link

are there any guide(-lines) for creating a PR? I have never done it before...

@TheJoeSchr
Copy link

ok, I got it working, I will try to make a PR tomorrow

@spawnia
Copy link
Collaborator

spawnia commented Mar 27, 2019

@JoeSchr https://github.com/nuwave/lighthouse/blob/master/.github/CONTRIBUTING.md

It is fine if you provide a WIP at first if the API is not fixed yet, but before merging anything we require tests and docs.

@TheJoeSchr
Copy link

TheJoeSchr commented Mar 28, 2019 via email

@TheJoeSchr
Copy link

done: #692

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants