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

authx - Add configuration screen #22058

Merged
merged 5 commits into from Feb 6, 2022
Merged

authx - Add configuration screen #22058

merged 5 commits into from Feb 6, 2022

Conversation

kainuk
Copy link
Contributor

@kainuk kainuk commented Nov 12, 2021

Overview

This extends the functionality of #19727 and adds a configuration screen. This will help with configuring for use with https://github.com/CiviMRF.

After

Selection_034

Screenshot from 2022-02-05 13-59-02

@civibot
Copy link

civibot bot commented Nov 12, 2021

(Standard links)

@civibot civibot bot added the master label Nov 12, 2021
@jaapjansma
Copy link
Contributor

test this please

@totten
Copy link
Member

totten commented Dec 11, 2021

jenkins, test this please

@totten
Copy link
Member

totten commented Dec 11, 2021

@kainuk Oooh, I really like that you added a configuration screen!

I notice this branch didn't bring over the description or the test-coverage from #19727. It makes sense -- the tests were still failing due to the X-Requested-With: XMLHttpRequest issue. I think maybe we should use #19727 to add the LegacyAuthenticator and its test-coverage -- then use #22058 to add the configuration screen. To that end....

(1) I've ported your fix for protected function login(AuthenticatorTarget $tgt) to #19727 and added a fix for XMLHttpRequest.

(2) I tried out the new config screen, and it was working. 👍 Kind of impressed that it worked with so little code. I haven't lookd at CRM_Admin_Form_Generic much. How hard would it be to either (a) display some of the help/description text for the settings and/or (b) arrange some of the settings in a table so that it's easier to skim.

@BettyDolfing
Copy link

test this please

@totten totten changed the title Lecacy auth authx - Configuration screen. Legacy authenticator. Dec 14, 2021
@BettyDolfing
Copy link

test this please

@totten totten changed the title authx - Configuration screen. Legacy authenticator. authx - Add configuration screen Feb 5, 2022
@totten
Copy link
Member

totten commented Feb 5, 2022

Rebased to resolve recent conflicts.

To address my own review-feedback, I added a few commits to tweak the settings. It still uses the generic settings_pages, and I don't know how to make the generic page present a matrix of options... but the ordering+sizing now have more consistent alignment, and there's some help-text.

Updated description to add a screenshot of the final page.

Merge on pass.

@totten totten force-pushed the lecacy-auth branch 2 times, most recently from 733275e to eafc90f Compare February 5, 2022 22:13
@totten
Copy link
Member

totten commented Feb 5, 2022

jenkins, test this please

Process note: Something quirky happened with Github. Contemporaneous with the previous comment, I force-pushed a very small copyedit (adding like 2-5 words; end commit eafc90f), but it didn't appear in web UI for 15+ min, and it didn't send a webhook.

@totten
Copy link
Member

totten commented Feb 5, 2022

Process note: Hrm, Github also hasn't sent the webhook for test this please.

Well, there's a test-run for a nearly identical commit https://test.civicrm.org/job/CiviCRM-Core-PR/46962/ which we can use instead...

@colemanw
Copy link
Member

colemanw commented Feb 6, 2022

It took a while but the test run eventually started

@totten totten merged commit 3a665ef into civicrm:master Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants