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.

Sign up for GitHub

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

Jump to bottom

Unsave post from saved tab #15502

Closed
wants to merge 4 commits into from
Closed

Unsave post from saved tab #15502

wants to merge 4 commits into from

Conversation

momo-ozawa
Copy link
Contributor

@momo-ozawa momo-ozawa commented Dec 15, 2020 •
edited

Fixes #15381

This PR addresses fixes the following:

  • When a user taps the save icon button for a post in the Saved tab, the post cell wasn't being updated to a ReaderSavedPostUndoCell
  • When a user taps the save icon button for a post in the Saved tab, the post wasn't being unsaved

To test:

  1. Go to the Reader tab
  2. Save a post from the Following tab
  3. Go to the Saved tab and unsave the post
    • The post cell should be updated to a ReaderSavedPostUndoCell ✅
  4. Go to the Following tab and pull down to refresh
    • The post should be no longer be marked as "saved" ✅
  5. Go to the Saved tab
    • The post should no longer appear in the Saved tab ✅

unsave

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@momo-ozawa
Add ReaderSavedPostCellActionsDelegate
7163dc1 
ReaderSavedPostCellActionsDelegate was deleted in #15414, but turns out we need it.
@momo-ozawa
Clear removed posts from Saved tab
8aad12a 
willSet isn’t called unless contentType is set explicitly.

As a result, unsaving a post in the Saved tab wasn’t executing toggleSavedForLater.
@momo-ozawa
Update toggleSavedForLater notice display logic
abf408d 
Don’t display a success/failure notice if a post is saved/unsaved from the savedStream
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 15, 2020 •
edited

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

@momo-ozawa momo-ozawa added this to the 16.4 ❄️ milestone Dec 15, 2020
@momo-ozawa momo-ozawa self-assigned this Dec 15, 2020
momo-ozawa
momo-ozawa commented Dec 15, 2020
View reviewed changes
WordPress/Classes/ViewRelated/Reader/ReaderSaveForLaterAction.swift
Comment on lines -35 to +43
self.presentSuccessNotice(for: post, context: context, origin: origin, completion: completion)
if origin == .otherStream {
self.presentSuccessNotice(for: post, context: context, origin: origin, completion: completion)
}
completion?()
}, failure: { error in
}, failure: { error in
if origin == .otherStream {
self.presentErrorNotice(error, activating: !post.isSavedForLater)
completion?()
}
completion?()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should show the success/failure notice if a user unsaves a post from the Saved tab for the following reasons:

  • The ReaderSavedPostUndoCell already indicates to the user that the post has been "unsaved"
  • The notice appears when a user navigates away from the Saved tab... so the notice seems out of context

Let me know what you think! 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. I think to the ReaderSavedPostUndoCell is sufficient.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 15, 2020 •
edited

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

@ScoutHarris
Copy link
Contributor

Hey @momo-ozawa .

I'm seeing two issues:

  1. When I tap 'View All' on the toast message, 'Saved' shows the posts from the stream I was on.

view_all

2 ) I save a post, go to 'Saved', unsave there, return to the original stream, pull to refresh, the post is still saved.

unsave_fail

@momo-ozawa momo-ozawa changed the title Fix/15381 unsave post from saved tab Unsave post from saved tab Dec 16, 2020
@momo-ozawa
Copy link
Contributor Author

Hi @ScoutHarris! Thanks so much for catching those issues! 🙏

When I tap 'View All' on the toast message, 'Saved' shows the posts from the stream I was on.

I made a dedicated issue for this bug 👍
#15514

I save a post, go to 'Saved', unsave there, return to the original stream, pull to refresh, the post is still saved.

Fixed! 6692d93
Tested on both iPad and iPhone.

unsave-ipad

ScoutHarris
ScoutHarris approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

Works as advertised!

:shipit:

@momo-ozawa
Copy link
Contributor Author

Should be merged into 16.4
Closing this PR in favor of #15526

@momo-ozawa momo-ozawa closed this Dec 16, 2020
@momo-ozawa momo-ozawa deleted the fix/15381-unsave-post-from-saved-tab branch December 16, 2020 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ScoutHarris ScoutHarris ScoutHarris approved these changes

@leandroalonso leandroalonso Awaiting requested review from leandroalonso

Assignees

@momo-ozawa momo-ozawa

Labels
New Reader iOS Reader Improvements iOS [Pri] Medium [Type] Bug
Projects
None yet
Milestone
16.4
Development

Successfully merging this pull request may close these issues.

Reader: cannot unsave post from Saved filter
2 participants
@momo-ozawa @ScoutHarris