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

Add the new identifier_first argument to prompts #187

Merged

Conversation

kpurdon
Copy link
Contributor

@kpurdon kpurdon commented Jan 21, 2021

Proposed Changes

Adds the new identifier_first argument to prompts. I will update the terraform provider as well once this lands and is tagged.

https://support.auth0.com/notifications/5fff0ab3f28427000a77941e

Acceptance Test Output

kpurdon@syndio: ~/.../kpurdon/auth0 go test ./... -v -run TestPrompt
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/client	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/tag	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/testing/expect	(cached) [no tests to run]
=== RUN   TestPrompt
=== RUN   TestPrompt/Read
    prompt_test.go:14: {
          "universal_login_experience": "classic"
        }
=== RUN   TestPrompt/Update
    prompt_test.go:48: {
          "universal_login_experience": "new",
          "identifier_first": true
        }
--- PASS: TestPrompt (1.67s)
    --- PASS: TestPrompt/Read (1.24s)
    --- PASS: TestPrompt/Update (0.42s)
PASS
ok  	gopkg.in/auth0.v5/management	1.956s

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

@kpurdon kpurdon force-pushed the kpurdon/add-prompt-identifier-first branch from f0aceed to aebcecd Compare January 21, 2021 19:23
@kpurdon
Copy link
Contributor Author

kpurdon commented Jan 21, 2021

cc @alexkappa

@kpurdon
Copy link
Contributor Author

kpurdon commented Feb 9, 2021

Hi @alexkappa checking in here, would love to get this and the terraform PR reviewed soon.

@@ -3,6 +3,9 @@ package management
type Prompt struct {
// Which login experience to use. Can be `new` or `classic`.
UniversalLoginExperience string `json:"universal_login_experience,omitempty"`

// IdentifierFirst determines if the login screen prompts for just the identifier, identifier and password first.
IdentifierFirst bool `json:"identifier_first,omitempty"`

Choose a reason for hiding this comment

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

Should this be *bool to follow the convention of fields in all the other structs? It would then better deal with null responses from the API.

Looks like the existing UniversalLoginExperience field in this struct should be updated too, but we can do that in another PR and discuss first (it'd be a breaking change).

If updated to *bool you should run go generate ./... to generate an accessor for the field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to *bool. I don't think the string field needs to be a pointer since an empty string will get handled by omitempty were false would not.

As a side note after running generate it looks like it generated a bunch of things from other PRs. It would be great to run everything up until the tests on every PR to ensure fmt/generate is good before merging to master.

@kpurdon kpurdon force-pushed the kpurdon/add-prompt-identifier-first branch 3 times, most recently from 7069be4 to dc37a5a Compare February 11, 2021 17:07
@kpurdon
Copy link
Contributor Author

kpurdon commented Feb 13, 2021

@paddycarey anything else I can do here before merging? Once this is merged/released I can update the terrafrom side. Thanks!

@kpurdon
Copy link
Contributor Author

kpurdon commented Mar 12, 2021

@alexkappa @paddycarey what can I do to get this merged?

@kpurdon kpurdon mentioned this pull request Mar 17, 2021
@kpurdon
Copy link
Contributor Author

kpurdon commented Mar 18, 2021

@alexkappa please let me know what we can do here.

@yvovandoorn
Copy link
Contributor

@kpurdon With the merging of #200, the generated accessors are conflicting. Could you re-run go generate and commit? I'll commit to getting this merged afterwards.

@kpurdon kpurdon force-pushed the kpurdon/add-prompt-identifier-first branch 2 times, most recently from 2af2c05 to acc0e65 Compare March 23, 2021 23:02
@kpurdon
Copy link
Contributor Author

kpurdon commented Mar 23, 2021

@yvovandoorn updated as requested. Thanks!

@yvovandoorn
Copy link
Contributor

Hey @kpurdon. Thanks for that!

Unfortunately the test for it isn't passing locally:

go test ./... -v -run TestPrompt                                                     ✔ │ 08:00:05  
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5	0.016s [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/client	0.010s [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/tag	0.015s [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/testing/expect	0.018s [no tests to run]
=== RUN   TestPrompt
=== RUN   TestPrompt/Read
    prompt_test.go:14: {
          "universal_login_experience": "new",
          "identifier_first": true
        }
=== RUN   TestPrompt/Update
    prompt_test.go:49: unexpected output. have 0xc00001c273, expected 0xc0000a2349
    prompt_test.go:51: {
          "universal_login_experience": "new",
          "identifier_first": true
        }
--- FAIL: TestPrompt (0.74s)
    --- PASS: TestPrompt/Read (0.45s)
    --- FAIL: TestPrompt/Update (0.29s)
FAIL
FAIL	gopkg.in/auth0.v5/management	0.749s
FAIL

Which got me to think on how you're testing. I see that you've imported the json library. Would you consider adopting the way booleans & strings are tested in other API endpoints (e.g. hook_test.go is a reasonable one to look at).

Rather than doing:

	falseV := false
	trueV := true

You can use auth0.Bool(false) and auth0.Bool(true).

Than rather than using literal strings (e.g. UniversalLoginExperience: "classic",) you'd use something like UniversalLoginExperience: auth0.String("classic"),.

Then you can use "gopkg.in/auth0.v5/internal/testing/expect" library and reduce the expects to something (like):

expect.Expect(t, r["UniversalLoginExperience"], "new")

which would be consistent with the other tests in this SDK.

@kpurdon kpurdon force-pushed the kpurdon/add-prompt-identifier-first branch 2 times, most recently from 08b633f to 0d85fde Compare March 24, 2021 14:52
@kpurdon
Copy link
Contributor Author

kpurdon commented Mar 24, 2021

@yvovandoorn updated the tests and confirmed they are passing locally

kpurdon@syndio: ~/.../kpurdon/auth0 go test ./... -v -run TestPrompt
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/client	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/tag	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/testing/expect	(cached) [no tests to run]
=== RUN   TestPrompt
--- PASS: TestPrompt (2.69s)
PASS
ok  	gopkg.in/auth0.v5/management	(cached)

@kpurdon kpurdon force-pushed the kpurdon/add-prompt-identifier-first branch from 0d85fde to c4f85c9 Compare March 24, 2021 14:53
@yvovandoorn
Copy link
Contributor

💥 🚀

@yvovandoorn yvovandoorn merged commit 43d7c23 into go-auth0:master Mar 24, 2021
@kpurdon kpurdon deleted the kpurdon/add-prompt-identifier-first branch March 24, 2021 14:57
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.

3 participants