Change how Advanced admin settings fields are output #429

joshcanhelp · 2018-04-10T22:09:09Z

This commit will:

Switch Advanced admin settings to use new render functions
Reword confusing and incorrect descriptions
Rename incorrect protected vars
Add check for function_exists to use hooks for settings callbacks
Fix settings order and option names

Switch Advanced admin settings to use new render functions. Reword confusing and incorrect descriptions. Renamed incorrect protected vars. Add check for function_exists to use hooks for settings callbacks.

lbalmaceda

Many of the functions modified here previously received no args. Shouldn't you have $args = {} as parameter for retro compatibility? (methods are public). I see this repeats along the PR. Please check them all if this applies.

lbalmaceda · 2018-04-12T21:09:10Z

lib/admin/WP_Auth0_Admin_Advanced.php

      array( 'name' => __( 'Force HTTPS Callback', 'wp-auth0' ), 'opt' => 'force_https_callback',
        'id' => 'wpa0_force_https_callback', 'function' => 'render_force_https_callback' ),
-      array( 'name' => __( 'Lock JS CDN URL', 'wp-auth0' ), 'opt' => 'passwordless_enabled',
+      array( 'name' => __( 'Lock JS CDN URL', 'wp-auth0' ), 'opt' => 'cdn_url',


can these 2 var renaming changes be considered breaking changes?

They were incorrect in a previous (unreleased) branch. More testing found the incorrect option name.

lbalmaceda · 2018-04-12T21:10:53Z

lib/admin/WP_Auth0_Admin_Advanced.php

-  <?php
+  public function render_jwt_auth_integration( $args ) {
+    $this->render_switch( $args[ 'label_for' ], $args[ 'opt_name' ] );
+    $this->render_field_description( __( 'This will enable the JWT Auth Users Repository override', 'wp-auth0' ) );


Previous string had a 's after JWT. Is this intentional? and btw, what does this string mean?

Yes, intentional. Sounded awkward.

JWT Auth is an unsupported plugin we put out a while ago. So this is saying if that plugin is installed and this option is on, use the "user repository" from the plugin.

lbalmaceda · 2018-04-12T21:14:38Z

lib/admin/WP_Auth0_Admin_Advanced.php

+  public function render_custom_signup_fields( $args ) {
+    $this->render_textarea_field( $args[ 'label_for' ], $args[ 'opt_name' ] );
+    $this->render_field_description(
+      __( 'Valid JSON for custom signup fields in the Auth0 signup form. ', 'wp-auth0' ) .


I'd use the same name as in the docs

~~custom~~ additional signup fields

lbalmaceda · 2018-04-12T21:16:25Z

lib/admin/WP_Auth0_Admin_Advanced.php

+  public function render_link_auth0_users( $args ) {
+    $this->render_switch( $args[ 'label_for' ], $args[ 'opt_name' ] );
+    $this->render_field_description(
+      __( 'Links accounts with the same e-mail address (emails must be verified)', 'wp-auth0' )


If possible, add a small sentence telling when an email is considered verified. Such as: "An email is verified when the user was created using a social provider that shared the account's email or when a user receives a verification email and proceeds with the verify flow."

Added this to the "Require Verified Email" setting and mentioned it here.

lbalmaceda · 2018-04-12T21:17:32Z

lib/admin/WP_Auth0_Admin_Advanced.php

+  public function render_auto_provisioning( $args ) {
+    $this->render_switch( $args[ 'label_for' ], $args[ 'opt_name' ] );
+    $this->render_field_description(
+      __( 'Create new users in the WordPress database when signups are off. ', 'wp-auth0' ) .


if here you refer to the "auth0 database signups" I'd clarify that:

Create new users in the WordPress database when signups are disabled in the auth0 database connection and an existing auth0 user logs in. Or something like that. It's not clearer my suggestion, sorry haha.

Not clear or correct :)

This is ... WordPress registrations are off, user is added to Auth0 and logs in, user is added to WordPress. I think what I have is about as straightforward as I can get (after a few revisions) but open to suggestions.

How can users be "added to wordpress" if "wordpress registrations are off"?

You add in Auth0, successfully login, then they are added to WP

Is that expected? I understand there are 2 different user databases, with duplicated info IMO. What's the point on keeping a "auth0 user" that would never be able to "create a wordpress account"?

Looking through how this operates ... the scenario here is that WP registrations are turned off so the Lock form hides the Signup tab and the WP registration form is off. This means that no one can register themselves in WP (users can still be added by an admin). Now, an Auth0 admin adds a user in Auth0 and provides them their username and password. If the setting is turned:

off (default) then the login is blocked (throw new WP_Auth0_RegistrationNotEnabledException(); in WP_Auth0_UsersRepo::create())

on then the user is created as usual and life continues

I'm not saying this is how it should work, just how it currently does. IMHO, this should at least be turned on by default, maybe just forced to be the case.

lbalmaceda · 2018-04-12T21:29:59Z

lib/admin/WP_Auth0_Admin_Advanced.php

+  public function render_connections( $args ) {
+    $this->render_text_field( $args[ 'label_for' ], $args[ 'opt_name' ] );
+    $this->render_field_description(
+      __( 'Connections the Auth0 login form should show (separate multiple with commas). ', 'wp-auth0' ) .


Connections that the ..

and then

separate multiple connections with commas

(Same as in the IP text)

lbalmaceda · 2018-04-12T21:31:49Z

lib/admin/WP_Auth0_Admin_Advanced.php

+    $this->render_field_description(
+      __( 'Connections the Auth0 login form should show (separate multiple with commas). ', 'wp-auth0' ) .
+      __( 'If this is empty, all active connections will be shown. ', 'wp-auth0' ) .
+      __( 'Connections listed here must already be active under Connections in your ', 'wp-auth0' ) .


dashboard/connections/social would be enabling those for the TENANT scope, but then each CLIENT must enable those too. Make sure this is clearer please.

lbalmaceda · 2018-04-12T21:32:50Z

lib/admin/WP_Auth0_Admin_Advanced.php

+    $this->render_switch( $args[ 'label_for' ], $args[ 'opt_name' ] );
+    $this->render_field_description(
+      __( 'Require new users to verify their email before logging in. ', 'wp-auth0' ) .
+      __( 'This will disallow logins from social connections that do not provide email (like Twitter)', 'wp-auth0' )


Are users even created when this case happens? i.e. I sign up using Twitter. I won't be able to log in ever??

Users are created if an email is present in their userinfo. If this is on and you sign in with Twitter, you're rejected, correct.

@joshcanhelp But is the "wordpress user" created in that case?

No. No user created, no login allowed

lbalmaceda · 2018-04-12T21:34:25Z

lib/admin/WP_Auth0_Admin_Generic.php

@@ -133,7 +137,7 @@ protected function render_switch( $id, $input_name, $expand_id = '' ) {
 		printf(
 			'<div class="a0-switch"><input type="checkbox" name="%s[%s]" id="%s" data-expand="%s" value="1"%s>
 			<label for="%s"></label></div>',
-			esc_attr( $this->option_name ),
+			esc_attr( $this->_option_name ),


Line 30 declares this

$options_name = $this->_option_name . '_' . strtolower( $id );

Why is this getting renamed to _option_name ? I'm not following that

Renamed in a previous PR but these functions were added after that.

lbalmaceda · 2018-04-12T21:37:51Z

lib/admin/WP_Auth0_Admin_Generic.php

 	 *
 	 * @return string
 	 */
-	protected function get_dashboard_link( $path = '' ) {
-		return sprintf( '<a href="https://manage.auth0.com/#/%s" target="_blank">%s</a>',
+	protected function get_dashboard_link( $path = '', $append_period = FALSE ) {


I don't think this is necessary or part of this method logic. The method who is calling it must be completing the format. BTW it's only used 2 times on this doc, both of which have a follow up text that can easily contain those ". " you are adding in here. On the contrary, this line would have made sense that it called this method passing true since it doesn't have a follow up text, but it's not. 😂

The description method auto-adds periods. This is mainly to not screw up translation too much. A translate-able text string that just starts with a period and a space is hard to re-use in any context.

Slept on it ... this is goofy. Removing.

A translate-able text string that just starts with a period and a space is hard to re-use in any context.

This is true 100%. But then you can split ". The text to be translated" in ". "and"The text to be translated"`, fixing both what I pointed out and the translation inconvenience.

But then you're not able to change the period, if needed. Tradeoffs 🤷‍♂️

How so?

"Text with placeholder on my right" . <placeholder var> . ". " . "Other translatable text".

Something like this would work fine. Both the first and last texts could be translated just fine. The dot is a dot, it will never change at least not if you want to keep the original redaction of the message.

joshcanhelp · 2018-04-13T15:16:01Z

@lbalmaceda - I saw your main review comment re: $args. Those functions are callbacks that always get that $args argument, comes from here:

https://github.com/auth0/wp-auth0/blob/master/lib/admin/WP_Auth0_Admin_Generic.php#L30

So that has always been passed, I just added another key, opt_name, and started using it. 👍

lbalmaceda · 2018-04-13T17:16:55Z

How was that passed, if I see the method's signature is changing??

See line 137 doesn't include the parameter but now after the changes, that same line now receives $args

What I meant is that whoever was calling that public function in the past, would find that broken after this change.

joshcanhelp · 2018-04-13T17:21:13Z

@lbalmaceda - The function is called by WP core, it's public so that can happen. No one should be calling these directly. I could make the parameter default [] but then I have to catch empty cases within, which doesn't really make sense in this context.

I see what you're saying here but if someone is re-using a render callback directly, they're definitely doing it wrong.

I guess I could just return if the array is empty to avoid a fatal but that would have to be in every function.

lbalmaceda

For the record. I don't agree changing those public methods signature, but a future PR will include docs over the methods to warn users who use them directly.

Change how Advanced admin settings fields are output

e9b7ba1

Switch Advanced admin settings to use new render functions. Reword confusing and incorrect descriptions. Renamed incorrect protected vars. Add check for function_exists to use hooks for settings callbacks.

joshcanhelp added the CH: Changed label Apr 10, 2018

joshcanhelp added this to the v3-Next milestone Apr 10, 2018

joshcanhelp added the Ready for Review label Apr 11, 2018

lbalmaceda suggested changes Apr 12, 2018

View reviewed changes

PR feedback; minor rewording

b6c1ab2

lbalmaceda approved these changes Apr 13, 2018

View reviewed changes

Adding render callback default parameter

c7b2588

joshcanhelp merged commit 283401f into dev Apr 13, 2018

joshcanhelp deleted the change-settings-fields-display branch April 13, 2018 18:08

joshcanhelp removed the Ready for Review label Apr 17, 2018

joshcanhelp mentioned this pull request Jun 5, 2018

Release 3.6.0 #475

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change how Advanced admin settings fields are output #429

Change how Advanced admin settings fields are output #429

joshcanhelp commented Apr 10, 2018

lbalmaceda left a comment

lbalmaceda Apr 12, 2018

joshcanhelp Apr 12, 2018

lbalmaceda Apr 12, 2018

joshcanhelp Apr 12, 2018

lbalmaceda Apr 12, 2018

lbalmaceda Apr 12, 2018

joshcanhelp Apr 13, 2018

lbalmaceda Apr 12, 2018

joshcanhelp Apr 12, 2018

lbalmaceda Apr 13, 2018

joshcanhelp Apr 13, 2018

lbalmaceda Apr 13, 2018

joshcanhelp Apr 13, 2018

lbalmaceda Apr 12, 2018

lbalmaceda Apr 12, 2018

lbalmaceda Apr 12, 2018

joshcanhelp Apr 12, 2018

lbalmaceda Apr 13, 2018

joshcanhelp Apr 13, 2018

lbalmaceda Apr 12, 2018

joshcanhelp Apr 12, 2018

lbalmaceda Apr 12, 2018

joshcanhelp Apr 12, 2018

joshcanhelp Apr 13, 2018

lbalmaceda Apr 13, 2018 •

edited

Loading

joshcanhelp Apr 13, 2018

lbalmaceda Apr 13, 2018

joshcanhelp commented Apr 13, 2018

lbalmaceda commented Apr 13, 2018

joshcanhelp commented Apr 13, 2018

lbalmaceda left a comment

Change how Advanced admin settings fields are output #429

Change how Advanced admin settings fields are output #429

Conversation

joshcanhelp commented Apr 10, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp commented Apr 13, 2018

lbalmaceda commented Apr 13, 2018

joshcanhelp commented Apr 13, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

lbalmaceda Apr 13, 2018 •

edited

Loading