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

Refactor Hydra token introspection #92

Merged
merged 17 commits into from
Mar 2, 2023
Merged

Conversation

patch0
Copy link
Contributor

@patch0 patch0 commented Dec 6, 2022

What's changed

  • Rewrite oauth_user helper into identifiable concern to use the Hydra Admin API library introduced in Add a GraphQL endpoint #134
  • Changed the super class of the GraphQL controller to match the other API calls.
  • Upped the number of memoized helpers allowed by rubocop... 😅

Points for consideration

  • When testing, the auth token has to be given in the headers, so I've added that in everywhere.

This moves the Hydra Admin API helper into a separate library, using
more idiomatic call for Faraday.

It does mean that when testing the auth token has to be given in the
headers.
@raspberrypiherokubot raspberrypiherokubot temporarily deployed to editor-api-p-refactor-h-tmge7v December 6, 2022 09:32 Inactive
@raspberrypiherokubot raspberrypiherokubot temporarily deployed to editor-api-p-refactor-h-z7fnub December 6, 2022 09:34 Inactive
@raspberrypigithubbot
Copy link

@raspberrypiherokubot raspberrypiherokubot temporarily deployed to editor-api-p-refactor-h-zxyaqp December 6, 2022 09:41 Inactive
@raspberrypigithubbot
Copy link

@raspberrypigithubbot
Copy link

@patch0 patch0 marked this pull request as ready for review February 28, 2023 21:38
@@ -25,3 +25,5 @@ RSpec/DescribeClass:
- "spec/graphql/queries/**"
- "spec/graphql/mutations/**"

RSpec/MultipleMemoizedHelpers:
Max: 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of upping this could you not refactor the tests a bit?

So in the images_spec you don't really need the image_filename and instead could do
let(:params) { { images: [fixture_file_upload('test_image_1.png', 'image/png')] } } etc as it's not referenced in any tests.

Same for the update spec. You could remove the default_component_params and put them straight into the params as they are not being used in the tests I don't think.

Unless we wanted it increased of course :)

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 think there are more places where we use a lot of memoized helpers. I don't feel to bad about upping the number. We should discuss this though..

Copy link
Contributor

Choose a reason for hiding this comment

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

@patch0 Yeah I'm fine with it being increased to 8. There's already a few PR's in need of that from your overly productive day yesterday 😅 Just some autocorrectable offences to fix!

Copy link
Contributor

@IzzySmillie IzzySmillie left a comment

Choose a reason for hiding this comment

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

🥇

@patch0 patch0 merged commit 15fc62f into main Mar 2, 2023
@patch0 patch0 deleted the refactor-hydra-introspection branch March 2, 2023 09:50
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.

4 participants