Skip to content
This repository has been archived by the owner on Feb 16, 2022. It is now read-only.

Add post_identities endpoint support #178

Merged
merged 3 commits into from
Dec 31, 2020

Conversation

jmaeso
Copy link
Contributor

@jmaeso jmaeso commented Dec 24, 2020

Opening PR since looks to be the only way of running tests. Pending to verify if the feature works

@jmaeso jmaeso marked this pull request as ready for review December 24, 2020 15:22
@jmaeso
Copy link
Contributor Author

jmaeso commented Dec 24, 2020

Sorry, I'm not sure how to run the Test action 😅

@jmaeso jmaeso mentioned this pull request Dec 24, 2020
@jmaeso jmaeso force-pushed the feature/account_linking branch 2 times, most recently from 62115fa to acdfcf1 Compare December 28, 2020 08:06
Copy link
Contributor

@alexkappa alexkappa left a comment

Choose a reason for hiding this comment

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

Hi @jmaeso, thank you for taking the time to put this together!

The code looks pretty good. I made a couple of suggestions on aligning it more with the conventions used throughout the codebase.

Once ready you can ping me again so I can test/merge.

Cheers,
Alex

@@ -260,6 +260,37 @@ func TestUser(t *testing.T) {
}
t.Logf("%v\n", us)
})

t.Run("Link", func(t *testing.T) {
ulAlice, err := m.User.Search(Query(`email:"alice@example.com"`))
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also create a couple users, that better represent linking between two identities. We can make a better example of linking Batman to Bruce Wayne or Heisenberg to Walter White 😄

Just remember to clean up once done

t.Cleanup(func() { 
	m.User.Delete(walter.GetID())
	m.User.Delete(heisenberg.GetID())
})

@jmaeso
Copy link
Contributor Author

jmaeso commented Dec 28, 2020

Good comments @alexkappa , they make sense, I'll address them ASAP.
This morning though I tried to test the implementation. Unfortunately doesn't work, it will need some changes/additions into the internals of the library:
The way the manager.Request() works is that reuses the request's body to unmarhsal the server's response and in this case, both the payloads are different and cannot be reused.
Not sure how would you address this point. Playing withRequestOption (not sure if would work)? A new Request method? Update the existent method to decouple request & response types?

@jmaeso jmaeso force-pushed the feature/account_linking branch from dd1a2c1 to 845010a Compare December 28, 2020 15:36
@jmaeso jmaeso force-pushed the feature/account_linking branch from 845010a to 74396f6 Compare December 28, 2020 15:40
@jmaeso
Copy link
Contributor Author

jmaeso commented Dec 30, 2020

ping @alexkappa
Last changes are tested on some personal script, so should pass the tests. This being said, if you have concerns about the chosen solution, feel free to discuss alternatives

@alexkappa
Copy link
Contributor

Thanks @jmaeso, sorry about the request/response limitations you are experiencing here. It's quite uncommon but happens from time to time the payload of the request being different than that of the response.

For now I would suggest calling the contents of RequestWithCustomResponse directly within the Link method. I can try to think of a way to make parsing the response easier in the future.

@jmaeso jmaeso force-pushed the feature/account_linking branch from 8d53dff to 1714804 Compare December 30, 2020 16:20
@jmaeso
Copy link
Contributor Author

jmaeso commented Dec 30, 2020

Comments applied @alexkappa

@alexkappa alexkappa merged commit 1714804 into go-auth0:master Dec 31, 2020
@alexkappa
Copy link
Contributor

Nicely done @jmaeso! I've tested locally and merged it in. Will publish a release shortly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants