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

DXCDT-416: Add tests for updating universal login branding #692

Merged
merged 4 commits into from
Mar 30, 2023

Conversation

ewanharris
Copy link
Contributor

🔧 Changes

Adds tests that exercise the auth0 ul show and auth0 ul update commands with the provided long hand flags. Also fixes some issues identified whilst writing these tests (will be pushed after PR has been opened)

The first test sets the expected values just incase they are not what is expected, these are also reset in the cleanup script.

📚 References

🔬 Testing

This PR mostly adds tests and the changed code is exercised by the added tests

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@ewanharris ewanharris requested a review from a team as a code owner March 30, 2023 12:27
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: +0.70 🎉

Comparison is base (016b232) 56.23% compared to head (9db44e2) 56.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
+ Coverage   56.23%   56.93%   +0.70%     
==========================================
  Files          89       89              
  Lines       11506    11506              
==========================================
+ Hits         6470     6551      +81     
+ Misses       4595     4509      -86     
- Partials      441      446       +5     

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ewanharris ewanharris force-pushed the DXCDT-416-universal-login-tests branch from 71ee57d to e159345 Compare March 30, 2023 12:57
@@ -213,14 +215,6 @@ func updateUniversalLoginCmd(cli *cli) *cobra.Command {
b.Font = &management.BrandingFont{URL: &inputs.CustomFontURL}
}

if b.Font != nil {
Copy link
Contributor Author

@ewanharris ewanharris Mar 30, 2023

Choose a reason for hiding this comment

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

From testing, this code is not required. The only case where b.Font is not nil is when inputs.CustomFontURL is not set due to the if on line 212.

Given that, we will always repeat exactly what the if statement on 212 achieves, i.e. if a --font option has been provided then set it

@@ -178,7 +178,7 @@ func updateUniversalLoginCmd(cli *cli) *cobra.Command {
b := &management.Branding{}
isAccentColorSet := len(inputs.AccentColor) > 0
isBackgroundColorSet := len(inputs.BackgroundColor) > 0
currentHasColors := current.Colors != nil
currentHasColors := current.GetColors() != nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this error is solely for a new tenant/tenant that hasn't touched the values (due to this API being a PATCH API so it's difficult to reset the values)

But on first run of auth0 ul in my tenant I got the below error, this repeated for LogoURL and FaviconURL, looking at the API calls in the UI a GET /api/v2/branding only returned a subset of data {"colors":{"primary":"#FF4F40"}} rather than a full object

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x1009f2604]

goroutine 1 [running]:
github.com/auth0/auth0-cli/internal/cli.Execute.func1()
	/Users/ewanharris/dev/git/auth0-cli/internal/cli/root.go:53 +0xb4
panic({0x101050900, 0x1017f6f60})
	/opt/homebrew/Cellar/go/1.20.2/libexec/src/runtime/panic.go:884 +0x1f4
github.com/auth0/auth0-cli/internal/cli.updateUniversalLoginCmd.func1(0x1400001a900?, {0x140001201c0?, 0x2?, 0x2?})
	/Users/ewanharris/dev/git/auth0-cli/internal/cli/universal_login.go:181 +0x1c4
github.com/spf13/cobra.(*Command).execute(0x1400001a900, {0x140001201a0, 0x2, 0x2})
	/Users/ewanharris/dev/git/auth0-cli/vendor/github.com/spf13/cobra/command.go:916 +0x5c4
github.com/spf13/cobra.(*Command).ExecuteC(0x14000292600)
	/Users/ewanharris/dev/git/auth0-cli/vendor/github.com/spf13/cobra/command.go:1044 +0x340
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/ewanharris/dev/git/auth0-cli/vendor/github.com/spf13/cobra/command.go:968
github.com/spf13/cobra.(*Command).ExecuteContext(...)
	/Users/ewanharris/dev/git/auth0-cli/vendor/github.com/spf13/cobra/command.go:961
github.com/auth0/auth0-cli/internal/cli.Execute()
	/Users/ewanharris/dev/git/auth0-cli/internal/cli/root.go:64 +0x1ac
main.main()
	/Users/ewanharris/dev/git/auth0-cli/cmd/auth0/main.go:6 +0x1c

@ewanharris ewanharris force-pushed the DXCDT-416-universal-login-tests branch from 8174159 to 31227f4 Compare March 30, 2023 14:01
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

Great work 👍

Co-authored-by: Will Vedder <[email protected]>
@willvedd willvedd enabled auto-merge (squash) March 30, 2023 14:12
@willvedd willvedd merged commit bb7f5a9 into main Mar 30, 2023
@willvedd willvedd deleted the DXCDT-416-universal-login-tests branch March 30, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants