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

Add oauth2 fields behind a feature flag #609

Closed
wants to merge 2 commits into from
Closed

Conversation

rdubrock
Copy link
Contributor

No description provided.

@rdubrock rdubrock requested a review from a team as a code owner September 14, 2023 17:34
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

I assume the backend isn't set up for this yet as I could see it in the request but didn't return any of the options I had added.

I left a comment about what my expectation of the "Use Oauth2" checkbox should do after you untick it but when it is ticked my other expectation is that some of the fields should be required at that point but they can all be sent empty?

Screenshot of Synthetic Monitoring plugin authentication accordion for http checks. The inputs are very  close to each other with not enough margin.

The UI is pretty cramped, could do with a bit of margin-bottom to separate the forms from the tickboxes.

disabled={!isEditor}
/>
</VerticalGroup>
<IconButton name="minus-circle" onClick={() => removeEndpointParam(index)} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a 'Delete' tooltip. (Same as the IconButton below). This button also isn't centred but top aligned.

A screenshot of the Synthetic Monitoring plugin with two input fields and the delete icon is aligned vertically to the top of the inputs.

`
}
value={includeOauth2}
onChange={() => setIncludeOauth2(!includeOauth2)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed when testing this my expectation of what this checkbox does versus what it is actually doing don't quite line up. If you tick this, start filling in the form, change your mind and untick it my expectation would be that none of the fields would be sent with the request but I can see that they are still.

It is worth noting, I think it is good to persist the fields in case someone accidentally unticks this and then wants to get back the information they have just added but I think it is a fair expectation that once they submit with this box unticked that any information within the oauth2 fields is lost.

size="sm"
disabled={!isEditor}
onClick={() => {
console.log('appending scope');
Copy link
Contributor

Choose a reason for hiding this comment

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

Left in by mistake? 😃

@rdubrock
Copy link
Contributor Author

Yeah this one is in limbo and I've been meaning to come back to it when I carve out some time. The backend supports it but might require some changes, and I need a good test case

@rdubrock
Copy link
Contributor Author

Closing this for now, this got a little too stale

@rdubrock rdubrock closed this Mar 28, 2024
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.

2 participants