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

.org Plans: Fix site slug in plan cancellation survey #9207

Merged
merged 2 commits into from
Nov 8, 2016

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Nov 8, 2016

This PR fixes #9200, where the site slug is not properly formatted and displays :: instead of / when the Jetpack site is installed in a subdirectory.

To test:

  • Checkout this branch
  • Purchase a plan for a Jetpack site which is installed in a subdirectory
  • Go through the process of canceling the plan
  • Verify the site slug is displayed with / and not with :: in the "Are you sure you want to remove {PLAN} from {SLUG}" text in the removal step
  • Verify the site slug is displayed with / and not with :: in the "{PLAN} was removed from {SLUG}" text in the removal confirmation notice.
  • Verify tests of the new slugToUrl method pass: npm run test-client client/lib/url/test/index.js -- --grep "slugToUrl"

/cc @beaulebens @johnHackworth

@tyxla tyxla added [Type] Bug When a feature is broken and / or not performing as intended Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. .org Plans labels Nov 8, 2016
@tyxla tyxla self-assigned this Nov 8, 2016
@matticbot
Copy link
Contributor

@roccotripaldi
Copy link
Member

Code looks solid, and tests are passing.
It took me a moment to find my testing site that is in a sub-folder. Testing now.

@roccotripaldi
Copy link
Member

LGTM!

@roccotripaldi roccotripaldi added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 8, 2016
@tyxla tyxla merged commit 67a7659 into master Nov 8, 2016
@tyxla tyxla deleted the fix/jetpack-plan-cancellation-survey-site-slug branch November 8, 2016 13:34
@@ -130,7 +131,7 @@ const RemovePurchase = React.createClass( {
notices.success(
this.translate( '%(productName)s was removed from {{siteName/}}.', {
args: { productName },
components: { siteName: <em>{ selectedSite.slug }</em> }
components: { siteName: <em>{ slugToUrl( selectedSite.slug ) }</em> }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is doing exactly the same as the domain attribute of the site entity, which is what should be used instead.

selectedSite.domain instead of slugToUrl( selectedSite.slug ).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, thanks for noticing @mtias 👍 Fixing in #9338.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Jetpack [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jetpack: Cancellation survey should encode URL properly
4 participants