jenkins-bot has submitted this change and it was merged. Change subject: Don't parse URL to manipulate query parameters ......................................................................
Don't parse URL to manipulate query parameters ... in MobileContext::doToggling. If the current request is a FauxRequest, then an exception is thrown when trying to access the request URL via FauxRequest#getFullRequestURL or #getRequestURL. While this is a violation of Liskov's Substitution Principle, which should be addressed, it's also true that MobileContext::doToggling shouldn't need to parse the current URL in order to manipulate its query parameters in order to redirect the UA. Include @jdlrobson's follow-on change, which makes the desktop to mobile to desktop view switching integration tests only run in the integration environment [0]. [0]: I3c1b2eade8adf38ebf27660a436e2df43cd3e09d Bug: T129600 Change-Id: I0a10706f31966f2ac9f5c1056b3cbebd70788478 Co-Author: jdlrobson <[email protected]> --- M includes/MobileContext.php A tests/browser/features/step_definitions/switch_views.rb A tests/browser/features/step_definitions/switch_views_bug_t129600.rb A tests/browser/features/support/pages/page.rb A tests/browser/features/switch_views.feature 5 files changed, 57 insertions(+), 3 deletions(-) Approvals: Jdlrobson: Looks good to me, approved Bmansurov: Looks good to me, but someone else must approve jenkins-bot: Verified diff --git a/includes/MobileContext.php b/includes/MobileContext.php index 641f41e..fbdc23f 100644 --- a/includes/MobileContext.php +++ b/includes/MobileContext.php @@ -958,9 +958,7 @@ return; } - $url = $this->getRequest()->getFullRequestURL(); - $parsed = wfParseUrl( $url ); - $query = isset( $parsed['query'] ) ? wfCgiToArray( $parsed['query'] ) : array(); + $query = $this->getRequest()->getQueryValues(); unset( $query['mobileaction'] ); unset( $query['useformat'] ); unset( $query['title'] ); diff --git a/tests/browser/features/step_definitions/switch_views.rb b/tests/browser/features/step_definitions/switch_views.rb new file mode 100644 index 0000000..5f289dc --- /dev/null +++ b/tests/browser/features/step_definitions/switch_views.rb @@ -0,0 +1,15 @@ +Given(/^I toggle the mobile view$/) do + on(Page).toggle_mobile_view +end + +Then(/^I should see the mobile view$/) do + expect(on(Page).toggle_view_desktop_element).to be_visible +end + +Given(/^I toggle the desktop view$/) do + on(Page).toggle_desktop_view +end + +Then(/^I should see the desktop view$/) do + expect(on(Page).toggle_view_mobile_element).to be_visible +end diff --git a/tests/browser/features/step_definitions/switch_views_bug_t129600.rb b/tests/browser/features/step_definitions/switch_views_bug_t129600.rb new file mode 100644 index 0000000..9fb87a6 --- /dev/null +++ b/tests/browser/features/step_definitions/switch_views_bug_t129600.rb @@ -0,0 +1,5 @@ +Given(/^I am on a page that transcludes content from a special page$/) do + api.create_page 'T129600', '{{Special:PrefixIndex/User:Admin/}}' + + step 'I am on the "T129600" page' +end diff --git a/tests/browser/features/support/pages/page.rb b/tests/browser/features/support/pages/page.rb new file mode 100644 index 0000000..be0d04b --- /dev/null +++ b/tests/browser/features/support/pages/page.rb @@ -0,0 +1,14 @@ +class Page + include PageObject + + a(:toggle_view_mobile, css: '.stopMobileRedirectToggle') + a(:toggle_view_desktop, id: 'mw-mf-display-toggle') + + def toggle_mobile_view + toggle_view_mobile_element.when_present.click + end + + def toggle_desktop_view + toggle_view_desktop_element.when_present.click + end +end diff --git a/tests/browser/features/switch_views.feature b/tests/browser/features/switch_views.feature new file mode 100644 index 0000000..0803014 --- /dev/null +++ b/tests/browser/features/switch_views.feature @@ -0,0 +1,22 @@ +@chrome @firefox @vagrant +Feature: Switch between mobile and desktop views + + @integration + Scenario: Switching from desktop view to mobile view + Given I am on the "Main Page" page + And I toggle the mobile view + Then I should see the mobile view + + @integration + Scenario: Switching from mobile view to desktop view + Given I am using the mobile site + And I am on the "Main Page" page + And I toggle the desktop view + Then I should see the desktop view + + @integration + Scenario: Bug: T129600 + Given I am on the desktop site + And I am on a page that transcludes content from a special page + And I toggle the mobile view + Then I should see the mobile view -- To view, visit https://gerrit.wikimedia.org/r/278729 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I0a10706f31966f2ac9f5c1056b3cbebd70788478 Gerrit-PatchSet: 7 Gerrit-Project: mediawiki/extensions/MobileFrontend Gerrit-Branch: master Gerrit-Owner: Phuedx <[email protected]> Gerrit-Reviewer: Bmansurov <[email protected]> Gerrit-Reviewer: Jdlrobson <[email protected]> Gerrit-Reviewer: Phuedx <[email protected]> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
