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

Remove Admin Authentication settings #75

Merged
merged 3 commits into from
Mar 1, 2023

Conversation

pcustic
Copy link
Contributor

@pcustic pcustic commented Feb 14, 2023

Description

This PR removes Admin authentication provider settings in the System configuration page.

Motivation and Context

We are removing Google OAuth authentication provider in the CM and with that all the external admin authentication provider code. Since that means we are also removing endpoints for admin auth edit we need to remove these options in the admin UI.

Notion
CM PR

How Has This Been Tested?

Existing tests passed. I'll try to run the admin UI locally.

Checklist:

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@pcustic pcustic marked this pull request as ready for review February 14, 2023 08:56
@pcustic
Copy link
Contributor Author

pcustic commented Feb 14, 2023

@jonathangreen @tdilauro @ray-lee I don't have access rights to assign people to PR so think of this as an PR review request :)

@io7m io7m requested a review from a team February 14, 2023 10:59
Copy link
Contributor

@io7m io7m left a comment

Choose a reason for hiding this comment

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

That looks removed to me. 👍

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just one small issue to resolve.

@@ -10,7 +10,6 @@ $fontfamily: 'Open Sans', sans-serif;
@import "colors";
@import "global";

@import "admin_auth_service_edit_form";
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like the corresponding admin_auth_service_edit_form.scss file has been removed. Do we need to keep that around for anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 👍 5764977

@pcustic pcustic requested review from io7m and tdilauro and removed request for io7m February 20, 2023 15:43
@pcustic
Copy link
Contributor Author

pcustic commented Feb 24, 2023

@tdilauro I can't merge this so if you feel this is good to go can you please merge it?
Also, the original PR in the CM is not reviewed/approved yet so I'm not sure if that should be reviewed first?

@jonathangreen jonathangreen merged commit f1b5d0f into ThePalaceProject:main Mar 1, 2023
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