-
Notifications
You must be signed in to change notification settings - Fork 111
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
App 6822: Adding CLI command to update the billing service #4612
Conversation
cli/client.go
Outdated
printf(cCtx.App.Writer, "City: %s", address.GetCity()) | ||
printf(cCtx.App.Writer, "State: %s", address.GetState()) | ||
printf(cCtx.App.Writer, "Postal Code: %s", address.GetZipcode()) | ||
printf(cCtx.App.Writer, "Country: %s", "USA") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we assume this actually? do we care if its USA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the same assumption as our Stripe payment assumptions:
- we can't charge people outside of the US because we haven't integrated Stripe's internal tax product yet.
I think we also can't pay people directly on stripe because of this?
I actually didn't mention this as a non-goal on the scope, but I can get confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a ticket to change it at the end of the epic as steve said its non-prio but it will actually require an API change
cli/app.go
Outdated
&cli.StringFlag{ | ||
Name: organizationBillingAddress, | ||
Required: true, | ||
Usage: "the stringified address that follows the pattern: line1, line2, city, state, zipcode", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or "line1, city, state, zipcode" right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, Ill add a line2 (optional) to the helper text
cli/utils.go
Outdated
currIndex := 0 | ||
|
||
addr.AddressLine_1 = splitAddress[currIndex] | ||
currIndex++ | ||
|
||
if len(splitAddress) == 4 { | ||
// if its only 4 lines long that means that there is no line2 | ||
currIndex++ | ||
} else { | ||
addr.AddressLine_2 = &splitAddress[currIndex] | ||
currIndex++ | ||
} | ||
|
||
addr.City = splitAddress[currIndex] | ||
currIndex++ | ||
addr.State = splitAddress[currIndex] | ||
currIndex++ | ||
addr.Zipcode = splitAddress[currIndex] | ||
return addr, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[style, optional] ik repeated code in this but might be more readable to do something like this:
currIndex := 0 | |
addr.AddressLine_1 = splitAddress[currIndex] | |
currIndex++ | |
if len(splitAddress) == 4 { | |
// if its only 4 lines long that means that there is no line2 | |
currIndex++ | |
} else { | |
addr.AddressLine_2 = &splitAddress[currIndex] | |
currIndex++ | |
} | |
addr.City = splitAddress[currIndex] | |
currIndex++ | |
addr.State = splitAddress[currIndex] | |
currIndex++ | |
addr.Zipcode = splitAddress[currIndex] | |
return addr, nil | |
if len(splitAddress) == 4 { | |
return &apppb.BillingAddress{ | |
Address_Line_1: splitAddress[0], | |
City: splitAddress[1], | |
State: splitAddress[2], | |
Zipcode: splitAddress[3], | |
}, nil | |
} | |
return &apppb.BillingAddress{ | |
Address_Line_1: splitAddress[0], | |
Address_Line_2: splitAddress[1], | |
City: splitAddress[2], | |
State: splitAddress[3], | |
Zipcode: splitAddress[4], | |
}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea thats what i had before, ill revert back to that 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few nits and looks like tests and stuff are failing but otherwise looks good. will approve with screenshot of manual test ?
The caveat here is that we need to pass in an address as a CLI command, IMO there are 2 possible ways to do this :
billing-service update --line1=<line-1> ---line-2=<line2> --city=<city> --state=<state> --zipcode=<zipcode>
billing-service update --address="line1, line2, city, state, zipcode
"I implemented 2 because I thought it was a better UX to the user but open to opinions here
Manual Testing: