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

Adding validation and 409 error to flags #1786

Merged
merged 25 commits into from
Sep 26, 2024

Conversation

RidhamShah
Copy link
Collaborator

@RidhamShah RidhamShah commented Jul 26, 2024

Updated Final Approach

Upon submission to the feature-flag functionality, the create/update API will be called. This API will return a 409 error if the key is not unique within the specified context (the combination of key and context must be unique). This error will be displayed in the UI as an error message beneath the key text box.

We will not take the snack-bar approach as the modal will always come above the snack-bar, and to solve that, many things need to be changed.
image

@RidhamShah RidhamShah self-assigned this Jul 26, 2024
@RidhamShah RidhamShah linked an issue Jul 26, 2024 that may be closed by this pull request
@zackcl
Copy link
Collaborator

zackcl commented Jul 26, 2024

I think it's better to not show the snackbar error while the modal is open.

Screenshot 2024-07-26 at 4 51 34 PM

@danoswaltCL
Copy link
Collaborator

I think there is somehow a lot of confusion about what was needed. This is the story:

When submitting a feature flag, either from POST or PUT, the key that is submitted should be unique per the requested context. If it is not, the user should see an error and be given the chance to correct it.

For implementation, 409 HTTP status should be sent back with a message to display in the snackbar.

"A flag with this key already exists for this app-context, please enter a unique key."

The ask was to create a uniqueness check for the existing endpoint, and to show a snackbar message on error.

@danoswaltCL
Copy link
Collaborator

There is a different story to add a validation endpoint for MVP2 if we wanted to, maybe that was what was what was being thought of, but this should essentially just be a simpler backend change to the existing endpoint.

Copy link
Collaborator

@danoswaltCL danoswaltCL left a comment

Choose a reason for hiding this comment

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

The pr doesn't match the requirements of the story.

@RidhamShah RidhamShah marked this pull request as draft July 29, 2024 05:25
@RidhamShah RidhamShah marked this pull request as ready for review July 29, 2024 08:12
@RidhamShah
Copy link
Collaborator Author

@danoswaltCL updated this PR as needed.
I also have commented out the form-validation endpoint (not removed) if we want to use Infuture or other versions.

@RidhamShah RidhamShah requested a review from danoswaltCL July 29, 2024 09:05
@Yagnik56
Copy link
Collaborator

@zackcl @danoswaltCL I tried to move snackbar above the modal but I can't find the class we can target to do that, maybe we can rethink the design on mvp2 for now we can go with this.

@danoswaltCL
Copy link
Collaborator

ok. let's talk in refinement, I want to hit pause on this story and talk as a team.

@danoswaltCL
Copy link
Collaborator

danoswaltCL commented Aug 2, 2024

@RidhamShah please see #1797 .

Once that is available, you'd add a new SERVER_ERROR enum for the duplicate key error, send that error from backend, and do the same steps as "EXAMPLE_ERROR" to create the observable. The form component can subscribe to that observable to show the error, and the errorEvent itself will have a method called clear() that you have to call when the error has been handled.

I'll update the story so that it says now that we do want to show this error in the form, and not in the snackbar, now that we have a better setup to do that.

@zackcl
Copy link
Collaborator

zackcl commented Sep 23, 2024

@zackcl Test/review this

@zackcl
Copy link
Collaborator

zackcl commented Sep 24, 2024

@RidhamShah Could we also make the Key field's outline red colored when the key is duplicated to emphasize that it is invalid?

Screenshot 2024-09-24 at 6 26 34 PM

This is also expected behavior for other fields when they're invalid:

Screenshot 2024-09-24 at 6 44 09 PM

@zackcl
Copy link
Collaborator

zackcl commented Sep 24, 2024

@RidhamShah I can confirm that the Key field's outline turns red when the field is invalid. However, I found a bug where if the user tries to update the Feature Flag's Name without updating the Key, we get a validation error in the modal.

Screenshot 2024-09-25 at 1 01 47 AM

This also happens when updating the Description. In this case, the Key field's outline is not red, which is strange.

Screenshot 2024-09-25 at 1 02 51 AM

Could you please make it possible to update a Feature Flag through the "Edit Feature Flag" modal while leaving the Key unchanged?

zackcl
zackcl previously requested changes Sep 24, 2024
Copy link
Collaborator

@zackcl zackcl left a comment

Choose a reason for hiding this comment

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

Please fix the bug I mentioned in the comment.

@RidhamShah
Copy link
Collaborator Author

Please fix the bug I mentioned in the comment.

Can you check now, updated the code.

@zackcl
Copy link
Collaborator

zackcl commented Sep 25, 2024

@RidhamShah I still can't update a feature flag while leaving the Key unchanged. Please see this video:

Cannot.update.flag.key.unchanged.mov

We should allow updating a feature flag while leaving the Key unchanged (e.g., only update Name or Description).

@Yagnik56
Copy link
Collaborator

Yagnik56 commented Sep 26, 2024

@zackcl can you try again? The issue is fixed!

@zackcl
Copy link
Collaborator

zackcl commented Sep 26, 2024

@Yagnik56 I can now update a feature flag while leaving the Key unchanged. However, the duplicated key error message no longer appears on the modal, and the error snackbar is visible again. Can we fix this?

@Yagnik56 Yagnik56 merged commit 4d06ff7 into dev Sep 26, 2024
14 checks passed
@Yagnik56 Yagnik56 deleted the feature/-1762-validation-and-409-error branch September 26, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FF Fix: Duplicate key feature flag 500 error
5 participants