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

Xpost Notifications: show Follow conversation by email on original post #17159

Merged
merged 8 commits into from
Sep 16, 2021

Conversation

ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented Sep 15, 2021

Fixes #17158
WPKit PR: wordpress-mobile/WordPressKit-iOS#446

When viewing the original post from an xpost Notification, the Follow conversation by email button now appears in the Comments header.

When the link is tapped in the Notification body, it now compares the post.crossPostMeta.postURL with the tapped URL. If they are the same, the linked post is shown using the IDs from post.crossPostMeta (i.e. the original post). This allows the endpoint to be called that returns subscribe information for the original post, thus having the necessary information to display the follow button.

Bonus: fixed issues that were causing Implicit conversion from enumeration type 'enum UIStackViewAlignment' to different enumeration type 'UIStackViewDistribution' (aka 'enum UIStackViewDistribution') warnings in ReaderPostHeaderView.

To test:

  • On a P2 you are following, ensure mobile notifications are enabled for new posts:

notif_settings

  • On a different P2, make a post cross posting to the P2 above.
  • In the app, go to Notifications.
  • Select the Notification for the xpost.
  • In the body of the Notification, tap the link to the original post.
  • Tap the Comments button at the bottom.
  • Verify the Follow(ing) conversation by email button appears in the header and shows the correct state.
Before After
Screen Shot 2021-09-15 at 11 38 39 AM Screen Shot 2021-09-15 at 4 36 49 PM

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Tested different Notification types to ensure posts are loaded correctly.

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 15, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@ScoutHarris ScoutHarris requested a review from aerych September 15, 2021 23:27
@ScoutHarris ScoutHarris marked this pull request as ready for review September 15, 2021 23:27
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 15, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

Hola @ScoutHarris 👋
Works great! :)
Tested as described (thanks for the tip on how to get the notification) and everything worked as expected. I just had a couple of nitpicky comments. :)

Comment on lines 499 to 504
// If the user tapped the link to the original post, use the original post's info to display reader detail.
// The API endpoint used by controllerWithPostID returns subscription flags for the post.
// The API endpoint used by controllerWithPostURL does return this information.
// These flags needed to display the `Follow conversation by email` option.
// So if we can call controllerWithPostID, do so. Otherwise, fallback to controllerWithPostURL.
// Ref: https://github.com/wordpress-mobile/WordPress-iOS/issues/17158
Copy link
Member

Choose a reason for hiding this comment

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

Should we preface this comment with something making it clear it's regarding cross posts?
We might be missing a word or two in these comments.
Should The API endpoint used by controllerWithPostURL does return this information read does not?
Should These flags needed to display the Follow conversation by email option say These flags are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What? You got the gist right? 😉 Thank you, I'll fix it.

Comment on lines +506 to +513
let readerDetail: ReaderDetailViewController = {
if let post = post,
selectedUrlIsCrossPost(url) {
return ReaderDetailViewController.controllerWithPostID(post.crossPostMeta.postID, siteID: post.crossPostMeta.siteID)
}

return ReaderDetailViewController.controllerWithPostURL(url)
}()
Copy link
Member

Choose a reason for hiding this comment

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

Nifty!

viewController?.navigationController?.pushViewController(readerDetail, animated: true)
}

private func selectedUrlIsCrossPost(_ url: URL) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

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

I'll be "that guy" and ask if we should add a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I suppose we could. 😄 I can do that in a follow up PR if you don't mind.

Copy link
Member

Choose a reason for hiding this comment

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

wfm!

@ScoutHarris
Copy link
Contributor Author

Hey @aerych .

I updated the comments. I'll do a follow up PR for the unit test, and I'll fix the merge conflict when I merge my WPKit change.

Back to you please!

@ScoutHarris ScoutHarris requested a review from aerych September 16, 2021 16:10
Copy link
Member

@aerych aerych left a comment

Choose a reason for hiding this comment

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

:shipit: with great 🚢 ing!

@ScoutHarris
Copy link
Contributor Author

Thanks @aerych !

@ScoutHarris ScoutHarris merged commit 2725b02 into develop Sep 16, 2021
@ScoutHarris ScoutHarris deleted the fix/xpost_follow_conversation branch September 16, 2021 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Xpost Notifications: Follow conversation by email not shown on original post
2 participants