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

Custom object import csv #3756

Merged

Conversation

brendanlaschke
Copy link
Contributor

@brendanlaschke brendanlaschke commented Feb 1, 2024

Adds the csv import for all objects.

closes #3247

@FelixMalfait
Copy link
Member

I don't think you missed anything important, this is the same direction I had in mind!

One challenge will be to handle creating relationships by passing an Id but we can do that in another issue/PR!
(Often people that are migrating have an existing list of people/companies so the workflow would probably be (1) import companies in Twenty (2) Export companies from Twenty to get the ID (3) attach the ID in my google sheet list of people with a vlookup (4) Import people)

@brendanlaschke
Copy link
Contributor Author

I don't think you missed anything important, this is the same direction I had in mind!

One challenge will be to handle creating relationships by passing an Id but we can do that in another issue/PR! (Often people that are migrating have an existing list of people/companies so the workflow would probably be (1) import companies in Twenty (2) Export companies from Twenty to get the ID (3) attach the ID in my google sheet list of people with a vlookup (4) Import people)

I think this shouldn't be a problem ... Do we validate these relation ids before the graphql insert ?

@brendanlaschke brendanlaschke changed the title Poc custom object import csv Custom object import csv Feb 1, 2024
@brendanlaschke
Copy link
Contributor Author

Im a bit confused figuring out the problem with the test - shouldn't the mocked response be matched? They seem to be the same ?

 Expected variables: 
{"data":[{"address":<undefined>,"annualRecurringRevenue":{"amountMicros":null,"currencyCode":"USD"},"domainName":"example.com","employees":null,"idealCustomerProfile":true,"linkedinLink":{"label":"linkedinLink","url":null},"name":"Example Company","xLink":{"label":"xLink","url":null},"id":"cb2e9f4b-20c3-4759-9315-4ffeecfaf71a"}]}
    
    Failed to match 1 mock for this query. The mocked response had the following variables:
{"data":[{"address":<undefined>,"annualRecurringRevenue":{"amountMicros":null,"currencyCode":"USD"},"domainName":"example.com","employees":null,"idealCustomerProfile":true,"linkedinLink":{"label":"linkedinLink","url":null},"name":"Example Company","xLink":{"label":"xLink","url":null},"id":"cb2e9f4b-20c3-4759-9315-4ffeecfaf71a"}]}
    
    This typically indicates a configuration error in your mocks setup, usually due to a typo or mismatched variable.

@FelixMalfait
Copy link
Member

Im a bit confused figuring out the problem with the test - shouldn't the mocked response be matched? They seem to be the same ?

Mmh I don't see the issue either! The only thing is with "address":<undefined>, maybe <undefined> != <undefined>?

I think this shouldn't be a problem ... Do we validate these relation ids before the graphql insert ?
Great!

We have foreign keys in DB so it will fail at that layer probably... (unless pg_graphql catches it before but I doubt it). It will force us to improve that kind of error handling, same problem with the API anyone can use already

@brendanlaschke
Copy link
Contributor Author

Apparently it was the undefined - thanks :)

@brendanlaschke brendanlaschke marked this pull request as ready for review February 5, 2024 23:01
@FelixMalfait
Copy link
Member

Great! I tested it and it works well!
Small feedbacks:

  1. non-blocking error
Screenshot 2024-02-06 at 09 11 22
  1. for composite field would probably be best to be consistent with other field types and not send them at all if they're empty
Screenshot 2024-02-06 at 09 14 55
  1. I agree with "TODO validate Relation Ids?", I don't think we should do a check on the object itself (server) but we could at least validate it's in a uuid format?

  2. We should only show the relationship where we have a foreign key on the target import table. So for example on the person table we should have companyId, but we should not have Attachments, Activities, Favorites... Because an activity belongsTo a company and we only want to import the object a company has in my opinion

…te fields, allow only import relations where toRelationMetadata
@brendanlaschke
Copy link
Contributor Author

Should be fixed!

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Perfect! Tested again and it works (including creating the relationship)
Thanks a lot!!!

@lucasbordeau lucasbordeau merged commit 7b8fffc into twentyhq:main Feb 6, 2024
9 of 10 checks passed
@brendanlaschke
Copy link
Contributor Author

@quest-bot loot #3247

@quest-bot quest-bot bot added the ⚔️ Quest Tracks quest-bot quests label Feb 6, 2024
Copy link

quest-bot bot commented Feb 6, 2024

Quest PR submitted! image Quest PR submitted!

@brendanlaschke You are attempting to solve the issue and loot this Quest. Will you be successful?


Questions? Check out the docs.

Copy link

quest-bot bot commented Feb 7, 2024

🧚 @brendanlaschke congratulations for completing Quest #3247

💰 A reward of $250 has been credited to you.

To claim your $250 reward follow the instructions here.

Questions? Check out the docs.

@brendanlaschke brendanlaschke deleted the poc-custom-object-import-csv branch February 9, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CSV import for any object
4 participants