-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactors Publicize connection URL detection #17816
Conversation
… or contained substrings
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
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.
Hey @frosty, the changes look great! I check for regressions by connecting each of the four (Facebook, LinkedIn, Tumblr, and Twitter) and all connected as expected.
I left some code comments, but no blockers, so feel free to merge as-is.
Following on from #17803, this PR refactors the code used to detect success or failure based on the authorization URL. In particular:
PublicizeConnectionURLMatcher
URLQueryItems
rather than just looking for substrings of the URL.To test
Regression Notes
The ability to connect any of the social accounts that are part of Publicize.
I added unit tests for the existing URL matching before I made any changes to that code, to verify that my changes didn't have any negative affect there.
See above. I also added extra tests around the authorize actions too. I added these at the end once I'd moved the authorize action code out of the view controller, as the VC had a bunch of dependencies making it a bit more awkward to test.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.