-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enabling Unsupported Block Editor for Jetpack sites with SSO enabled. #12606
Enabling Unsupported Block Editor for Jetpack sites with SSO enabled. #12606
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APK here. |
This PR is currently not ready for review as both test cases are not working. |
…k-sso-enabled # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
Hey! I'm moving this to 15.6 since 15.5 has been cut. I see this is marked as |
…k-sso-enabled # Conflicts: # libs/gutenberg-mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had an issue with self-hosted: WordPress/gutenberg#24476 (review)
…k-sso-enabled # Conflicts: # libs/gutenberg-mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested on a Jurassic Ninja instance connected via Jetpack and an Atomic site.
In both cases this feature is working as expected! 🎉
For the Jetpack case, I needed to change the web-editor preference manually. This is because of an issue that is external to us (so is the best we can do at this moment).
cc @guarani
Could someone check out the code side of the PR?
@mchowning or @cameronvoell perhaps?
Thanks, Eduardo! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried again with a new Jurassic Ninja test site using build 71274 and things worked fine. FWIW, the only difference here was that the WordPress.com account I connected with the site was not an Automattic account.
For the Jetpack case, I needed to change the web-editor preference manually. This is because of an issue that is external to us (so is the best we can do at this moment).
I didn't need to change any web-editor preference, it just worked 🤷♂️.
I re-tested Atomic and it's also working well.
Thanks for testing @guarani !
Nice to know that it works with a different account. Is this account newer than December 2018? (Is your a8c account older than December 2018 anyway?) |
Is the code side of this PR reviewed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a couple of small-ish code comments. Otherwise, the code looks good to me.
String token = bundle.getString(ARG_AUTHENTICATION_TOKEN); | ||
boolean isSitePrivate = bundle.getBoolean(ARG_IS_SITE_PRIVATE, false); | ||
|
||
boolean isSitePrivate = !gutenbergWebViewAuthorizationData.isWPCom(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is isSitePrivate
an accurate name for this variable? Would something like isSelfHosted
or !isWpCom
be more precise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this one. You are right. isSitePrivate
isn't an accurate name.
Changed to isSelfHosted
.
@@ -427,7 +403,9 @@ public void onActivityResult(int requestCode, int resultCode, @Nullable Intent d | |||
FragmentActivity activity = getActivity(); | |||
|
|||
Bundle arguments = getArguments(); | |||
boolean supportStockPhotos = arguments != null && arguments.getBoolean(ARG_SITE_USING_WPCOM_REST_API); | |||
GutenbergWebViewAuthorizationData gutenbergWebViewAuthorizationData = | |||
arguments.getParcelable(ARG_GUTENBERG_WEB_VIEW_AUTH_DATA); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for arguments
being null
here? The next two lines that use the arguments do that with arguments != null && ...
. Obviously we can't do that here since it's not a boolean, but I think we could achieve that effect if we inline this variable where it is used on line 409, since that is a boolean and has the null check.
There are actually a few places where we don't null check getArguments()
in this class. I actually don't feel strongly about null-checking getArguments()
because they should never be null as long as they are set (which we do), but I do think we should be consistent--either we always check them or we never do. Having a mix and sometimes checking them seems confusing. We seem closer to always checking them, and that's obviously a bit safer, so that would be my preference but, like I said, I don't feel particularly strongly (particularly if there is no reasonable way to handle a particular case where getArguments()
is null). WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mchowning! Good point here. We should pick a direction and probably we are closer to check getArguments()
to not be null.
Fixed.
Hey @mchowning 👋 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice work @marecar3 !
Thanks, @mchowning! cc @etoledom this one is ready for merge when the time comes. |
Details can be found on this PR: WordPress/gutenberg#24476
PR submission checklist:
RELEASE-NOTES.txt
if necessary.