Skip to content

Comments

Register and Update OAuth client via Stern/Backoffice#3305

Merged
battermann merged 3 commits intodevelopfrom
battermann/oauth-client-api-stern
May 22, 2023
Merged

Register and Update OAuth client via Stern/Backoffice#3305
battermann merged 3 commits intodevelopfrom
battermann/oauth-client-api-stern

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented May 19, 2023

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label May 19, 2023
@battermann battermann marked this pull request as ready for review May 19, 2023 10:29
@battermann battermann requested a review from fisx May 19, 2023 10:29
req <- baseRequest user Brig Unversioned $ "i/oauth/clients/" <> clientId
applicationName <- asString name
redirectUrl <- asString url
submit "POST" (req & addJSONObject ["application_name" .= applicationName, "redirect_url" .= redirectUrl])
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be PUT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess you are right. :)

case statusCode r of
200 -> parseResponse (mkError status502 "bad-upstream") r
404 -> throwE (mkError status404 "bad-upstream" "not-found")
_ -> throwE (mkError status502 "bad-upstream" "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_ -> throwE (mkError status502 "bad-upstream" "")
_ -> throwE (mkError status502 "bad-upstream" (show r))

(not tested; not that important, but might save some time if it breaks.)

test s "GET i/user/meta-info?id=..." testGetUserMetaInfo,
test s "/teams/:tid/search-visibility" testSearchVisibility
test s "/teams/:tid/search-visibility" testSearchVisibility,
test s "i/oauth/clients" testCrudOAuthClient
Copy link
Contributor

Choose a reason for hiding this comment

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

why the /i? stern doesn't have any public-facing api, so the distinction doesn't really work here i think.

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've seen it on other stern endpoints, too, so I adapted it. I guess, it just serves as a marker to make it obvious that this is calling an internal endpoint. Should I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

when i complained about this i thought this was the only end-point in stern with an /i, and that stern is only calling internal end-points. both assumptions may be wrong. use your own judgement, if it's consistent with what stern does everywhere, keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i see i'm a bit late. :) either way it's not a big deal.

]

testCrudOAuthClient :: TestM ()
testCrudOAuthClient = do
Copy link
Contributor

Choose a reason for hiding this comment

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

i won't insist, but maybe add new stern tests to /integration as well in the future? it's also good to save time, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. Now the work is done. Let's keep it for now.

@battermann battermann merged commit 4568341 into develop May 22, 2023
@battermann battermann deleted the battermann/oauth-client-api-stern branch May 22, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants