chore: show human readable errors for toggle flag and fetch environment #170

sunnyguduru · 2024-04-11T23:50:26Z

internal/quickstart/messages.go

internal/quickstart/toggle_flag.go

internal/quickstart/messages.go

sunnyguduru · 2024-04-12T18:30:18Z

gif added to description

dbolson

I think something like this setup will be simpler so we don't need to pass a boolean flag in the message.

…ssages

sunnyguduru · 2024-04-15T19:01:39Z

internal/quickstart/messages.go

@@ -117,6 +136,7 @@ func createFlag(client flags.Client, accessToken, baseUri, flagName, flagKey, pr
 					),
 				}
 			}
+			existingFlag = true


I realized that with the recent changes #179 we were now forgetting to set existingFlag when there is a createFlag conflict

sunnyguduru · 2024-04-15T19:05:03Z

internal/quickstart/toggle_flag.go

@@ -94,6 +97,9 @@ func (m toggleFlagModel) View() string {

 	if m.flagWasEnabled {
 		furtherInstructions = fmt.Sprintf("\n\nCheck your %s to see the change!", logTypeMap[m.sdkKind])
+		if m.alreadyEnabled {
+			furtherInstructions = "\n\nFlag was toggled too quickly."


Removed toggle state. Also followed the example from line 70 Error toggling flag: you have toggled too quickly.

dbolson · 2024-04-15T19:25:22Z

internal/quickstart/messages.go

@@ -25,10 +25,14 @@ func sendErrMsg(err error) tea.Cmd {
 	}
 }

-type toggledFlagMsg struct{}
+type toggledFlagMsg struct {
+	alreadyEnabled bool


I'm not sure this is more clear than passing an error and letting the model handle it. The createFlag command decides a particular error response isn't an actual error while the toggleFlag command always returns an error.

If the toggleFlag command returns either success (toggledFlagMsg) or failure (errMsg), then the model can decide how to show the error message instead of whether it's an error or not like the createFlag command.

That's fair. I'll update to follow that format

dbolson · 2024-04-15T21:36:48Z

internal/quickstart/messages.go

+				return errMsg{err: err}
+			}
+
+			if !msgRequestErr.IsConflict() {


I don't think we can get a conflict error when fetching the environment, so we're probably fine to just return the error every time.

dbolson · 2024-04-15T21:49:58Z

internal/quickstart/messages.go

+				return errMsg{err: err}
+			}
+
+			if !msgRequestErr.IsConflict() {


I don't think we need this since we're checking for this in the model.

dbolson

I get this message when I try to use "new" for the flag name:

invalid character 'E' looking for beginning of value

I also see "Error toggling flag: you have toggled too quickly" occasionally in addition to "Flag was toggled too quickly." Can you manually check these fixes?

sunnyguduru · 2024-04-15T23:08:27Z

I get this message when I try to use "new" for the flag name:

invalid character 'E' looking for beginning of value

I also see "Error toggling flag: you have toggled too quickly" occasionally in addition to "Flag was toggled too quickly." Can you manually check these fixes?

Yeah, I can look into that. Weird I didn't see the E error with Sunny test flag.

sunnyguduru · 2024-04-15T23:15:29Z

I get this message when I try to use "new" for the flag name:

invalid character 'E' looking for beginning of value

I also see "Error toggling flag: you have toggled too quickly" occasionally in addition to "Flag was toggled too quickly." Can you manually check these fixes?

The first issue is related to footerView. Seems that we were originally displaying errors there. I think it seems valuable to continue to show errors through footer view. So I want to remove showing errors through the "further instructions" string

sunnyguduru · 2024-04-15T23:19:39Z

I get this message when I try to use "new" for the flag name:

invalid character 'E' looking for beginning of value

I also see "Error toggling flag: you have toggled too quickly" occasionally in addition to "Flag was toggled too quickly." Can you manually check these fixes?

The first issue is related to footerView. Seems that we were originally displaying errors there. I think it seems valuable to continue to show errors through footer view. So I want to remove showing errors through the "further instructions" string

Although the spacing gets more confusing that way

sunnyguduru

here's a gif of me testing the changes:

Errors show up correctly for create flag (as well as conflict)
Errors show up correctly for toggle flag

sunnyguduru · 2024-04-16T22:59:55Z

internal/quickstart/create_flag.go

-		if m.existingFlagUsed {
-			successMessage = fmt.Sprintf("Using existing flag %q for setup.", m.flag.name)
-		}


This code path doesn't get run anymore as of PR #179

Seems like that's a bug. Can you add existingFlag = true in the createFlag command after checking if !msgRequestErr.IsConflict()? That would mean there is an error and it's a conflict error, so we want to set the variable to true.

sunnyguduru · 2024-04-16T23:00:22Z

internal/quickstart/messages.go

@@ -122,7 +119,7 @@ func createFlag(client flags.Client, accessToken, baseUri, flagName, flagKey, pr
 		return createdFlagMsg{flag: flag{
 			key:  flagKey,
 			name: flagName,
-		}, existingFlagUsed: existingFlag}


exisitingFlagUsed wasn't getting used anymore so I removed references to it.

sunnyguduru · 2024-04-16T23:02:27Z

internal/quickstart/toggle_flag.go

-	if m.flagWasEnabled {
+	if m.flagWasEnabled && m.err == nil {
 		furtherInstructions = fmt.Sprintf("\n\nCheck your %s to see the change!", logTypeMap[m.sdkKind])
 	}


Prevent showing the success message when there is an error.

…ssages

add human readable errors for toggle flag and fetch environment

d641792

sunnyguduru requested review from dbolson and k3llymariee April 11, 2024 23:50

dbolson reviewed Apr 12, 2024

View reviewed changes

internal/quickstart/messages.go Outdated Show resolved Hide resolved

Sunny Guduru added 3 commits April 12, 2024 10:36

handle conflict for toggleFlag

25a20a1

make helper function

7de9886

used handle error on fetchEnv

056fb7d

sunnyguduru commented Apr 12, 2024

View reviewed changes

internal/quickstart/toggle_flag.go Outdated Show resolved Hide resolved

internal/quickstart/messages.go Outdated Show resolved Hide resolved

internal/quickstart/messages.go Outdated Show resolved Hide resolved

sunnyguduru requested a review from dbolson April 12, 2024 18:21

dbolson reviewed Apr 15, 2024

View reviewed changes

Sunny Guduru added 6 commits April 15, 2024 11:34

Merge branch 'main' into sunny/sc-239301/show-human-friendly-error-me…

feb5003

…ssages

remove handleError. Remove unneeded parameters on message types

39bef6c

mend

3c7b4a8

add back parameter to createFlag

d83b3f4

added back parameter for toggle flag

66a0f92

rename variable

3a11a7e

sunnyguduru commented Apr 15, 2024

View reviewed changes

changed message to remove toggle state

2a67d50

sunnyguduru commented Apr 15, 2024

View reviewed changes

sunnyguduru requested a review from dbolson April 15, 2024 19:05

dbolson reviewed Apr 15, 2024

View reviewed changes

Sunny Guduru added 2 commits April 15, 2024 13:50

update create flag

92f3bfb

update toggle flag

bbd48f6

sunnyguduru requested a review from dbolson April 15, 2024 20:51

dbolson reviewed Apr 15, 2024

View reviewed changes

Sunny Guduru added 2 commits April 15, 2024 15:17

undo changes for fetchEnv

653a2a5

remove conflict handling on toggle flag cmd

b63f681

sunnyguduru requested a review from dbolson April 15, 2024 22:20

dbolson reviewed Apr 15, 2024

View reviewed changes

Sunny Guduru added 2 commits April 15, 2024 16:22

don't show duplicate error on toggle flag

6e7c486

show error peroperly on create flag

1a14357

sunnyguduru requested a review from dbolson April 15, 2024 23:25

Sunny Guduru added 3 commits April 16, 2024 15:23

fixed create flag

cefa5ca

fixed toggle flag

9ea827c

remove unused variable from toggle flag

75443f6

sunnyguduru commented Apr 16, 2024

View reviewed changes

Sunny Guduru added 3 commits April 16, 2024 16:24

Merge branch 'main' into sunny/sc-239301/show-human-friendly-error-me…

9453365

…ssages

fix bug

957e4b9

Merge branch 'main' into sunny/sc-239301/show-human-friendly-error-me…

b41e87f

…ssages

dbolson approved these changes Apr 18, 2024

View reviewed changes

sunnyguduru merged commit 4e812c6 into main Apr 18, 2024
2 checks passed

sunnyguduru deleted the sunny/sc-239301/show-human-friendly-error-messages branch April 18, 2024 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: show human readable errors for toggle flag and fetch environment #170

chore: show human readable errors for toggle flag and fetch environment #170

sunnyguduru commented Apr 11, 2024 •

edited

Loading

sunnyguduru commented Apr 12, 2024

dbolson left a comment

sunnyguduru Apr 15, 2024

sunnyguduru Apr 15, 2024

dbolson Apr 15, 2024

sunnyguduru Apr 15, 2024

dbolson Apr 15, 2024

dbolson Apr 15, 2024

dbolson left a comment •

edited

Loading

sunnyguduru commented Apr 15, 2024

sunnyguduru commented Apr 15, 2024

sunnyguduru commented Apr 15, 2024

sunnyguduru left a comment

sunnyguduru Apr 16, 2024

dbolson Apr 16, 2024

sunnyguduru Apr 16, 2024

sunnyguduru Apr 16, 2024

chore: show human readable errors for toggle flag and fetch environment #170

chore: show human readable errors for toggle flag and fetch environment #170

Conversation

sunnyguduru commented Apr 11, 2024 • edited Loading

sunnyguduru commented Apr 12, 2024

dbolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolson left a comment • edited Loading

Choose a reason for hiding this comment

sunnyguduru commented Apr 15, 2024

sunnyguduru commented Apr 15, 2024

sunnyguduru commented Apr 15, 2024

sunnyguduru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnyguduru commented Apr 11, 2024 •

edited

Loading

dbolson left a comment •

edited

Loading