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

Add ability to Unskip or Pull items from the Skipped tab on the Pull screen #728

Merged
merged 3 commits into from
Mar 10, 2021

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Mar 2, 2021

Description of the Change

At the moment, if you Skip an item from the Pull screen, that item goes to a Skipped tab. From here, the only option a user has is to view that item. So if someone were to accidentally Skip an item or change their mind later, there isn't an easy way for them to later pull that item.

This PR adds the same Pull options to the Skipped tab, both in the bulk actions and in the inline row options. This allows someone to directly Pull an item from the Skipped tab.

In addition, it adds a new Unskip option, both in the bulk actions and in an inline row option. If this Unskip option is used, the Skipped item is sent back to the New screen and can then be Pulled from there later (or Skipped again if desired).

Note: by default, we show the 10 most recent items on the new screen. If someone were to Unskip an item that doesn't belong to one of those 10 items, this item will still be Unskipped but won't show up on the first page of results. Pagination will have to be used to find this item.

Alternate Designs

None

Benefits

Users can now Unskip or directly Pull previously Skipped items

Possible Drawbacks

None

Verification Process

  1. Go to the Pull screen and from the New tab, Skip an item
  2. Go to the Skipped tab and find this Skipped item
  3. Test using the inline row option and Unskip this item
  4. Verify that item now shows back up in the New tab
  5. Repeat using the Bulk option
  6. In addition, test if the Pull option works as expected from the Skipped tab following the same directions as above but using the Pull option instead of the Unskip option

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Addresses part of #392

Darin Kotter added 2 commits March 2, 2021 15:21
…ems. Add necessary processing to properly unskip an item. Add helper functions to get a sync log from a connection. Add ability to overwrite the entire sync log for a connection
@dkotter dkotter self-assigned this Mar 2, 2021
@jeffpaul jeffpaul added this to the 1.6.3 milestone Mar 3, 2021
* @since 0.8
*/
public function log_sync( array $item_id_mappings, $blog_id = 0 ) {
public function log_sync( array $item_id_mappings, $blog_id = 0, $overwrite = false ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jeffpaul I think the logic for $overwrite is missing from this function. Currently the post ID is set to false which means it still shows on the skipped screen.

If an empty array is passed to $item_id_mappings it will fallback to $current_site_log instead, but it should overwrite.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@elliott-stocks Ah, nice catch. I had refactored this a bit and in the process I ended up breaking things. I believe it should be working properly now

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!

@jeffpaul jeffpaul requested a review from elliott-stocks March 5, 2021 01:52

$connection->log_sync( $sync_log, intval( $_GET['connection_id'] ), true );

setcookie( 'dt-unskipped', 1, time() + DAY_IN_SECONDS, ADMIN_COOKIE_PATH, COOKIE_DOMAIN, is_ssl() );
Copy link
Contributor

Choose a reason for hiding this comment

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

@dkotter VIP will flag this due to caching, although as it's in the admin it may not be an issue. Testing locally on VIP seems fine, but it may fail PHPCS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I debated changing how this works but this is how we already handle skipped or pulled items, by setting a cookie. So I decided it was best to just follow the pattern we already have in place.

@elliott-stocks elliott-stocks self-requested a review March 8, 2021 12:51
@elliott-stocks
Copy link
Contributor

@dkotter @jeffpaul This works well locally - approved.

Added a note about the cookie logic as this will be flagged by VIP, but as it's in the admin I think it should be fine.

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

Tested and this is looking good to me 👍🏼

@jeffpaul jeffpaul merged commit 97f37b0 into develop Mar 10, 2021
@jeffpaul jeffpaul deleted the feature/pull-unskip branch March 10, 2021 02:29
@jeffpaul jeffpaul mentioned this pull request Mar 10, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants