Media Library #3: Deletion by frosty · Pull Request #6858 · wordpress-mobile/WordPress-iOS

frosty · 2017-03-10T15:07:54Z

This PR adds the ability to delete media items from the Media Library. There are two ways to do it:

View the detail screen for any media item and tap the trashcan in the navigation bar. Tap the Delete button, and the item will be deleted.
On the media library grid screen, tap the Edit button in the navigation bar, and then select a number of media items. Tap the trashcan to delete them.

Note: This PR currently references a MediaPicker-iOS branch which contains a couple of necessary tweaks. Once that's approved, I'll push out a new pod version and update this branch accordingly.

To test:

Perform the steps above. Check on both dotcom and self-hosted.
Check the collection view and navigation bar update their state correctly as you enter and exit from Edit mode.
Check nothing crashes if you delete an item whilst its full size image is still loading in the detail view.

Needs review: @kurzee

SergioEstevao · 2017-03-10T15:12:18Z

WordPress/Classes/Services/MediaService.m

+
+    [mediaObjects enumerateObjectsUsingBlock:^(Media *media, NSUInteger idx, BOOL *stop) {
+        dispatch_group_enter(group);
+        [self deleteMedia:media success:^{


This looks like case to reconsider implementing back support for batch calls in the REST API . :)

Did we have it previously?

Yes we did, it was removed when the API code was updated to swift and URLSession.
Maybe add a issue for us to have a look in the future.

kurzee · 2017-03-10T21:45:51Z

One scenario:

Open Media, select an item for the detail.
Open Editor, Media Picker, select Media Library.
Go to the web, delete the same Media item selected in the detail on step 1.
In the app, refresh Editor's Media picker while it's open. The item is deleted from both the picker and the Media Library behind the views.
Close the editor modals. Notice the detail is still visible.

It didn't crash! But, it would be cool if we could reliably pop the detail view. I also imagine it would crash if proceeding on to editing details.

kurzee · 2017-03-10T22:14:04Z

Very similar to the case in #6858 (comment)

Open Media, select Edit and select an item.
Open Editor, Media Picker, select Media Library.
Go to the web, delete the same Media item selected in step 1.
In the app, refresh Editor's Media picker while it's open. The item is deleted from both the picker and the Media Library behind the views.
Close the editor modals. Notice the nav bar still show the enable Trash bar button item and cancel.

kurzee · 2017-03-10T22:22:31Z

Looks like the Cancel bar button item stays enabled during the deletion of items, as does the trash button.

kurzee

@frosty on first pass this works really well! Just a few code comments and edge cases to take a look at handling.

kurzee · 2017-03-10T21:47:41Z

WordPress/Classes/Networking/MediaServiceRemoteREST.m

+
+    [self.wordPressComRestApi POST:requestUrl
+                        parameters:nil
+                           success:^(id responseObject, NSHTTPURLResponse *response) {


Should we check the API response for "status": "deleted" as seen on https://developer.wordpress.com/docs/api/1.1/post/sites/%24site/media/%24media_ID/delete/?

Our delete post code doesn't seem to check the response, but I guess it couldn't hurt to do so. I've updated the remotes code to do this. I wasn't really sure what sort of error to pass if the response wasn't what we expected. Any suggestions? Is what I've done okay?

While a bit painful, probably best go ahead and knock it out now with something like https://github.com/wordpress-mobile/WordPress-iOS/blob/develop/WordPress/Classes/Services/MediaService.m#L339 and a message of "Something went wrong, please try again" or the like.

kurzee · 2017-03-10T21:51:14Z

WordPress/Classes/Networking/MediaServiceRemoteXMLRPC.m

+    NSParameterAssert([media.mediaID longLongValue] > 0);
+
+    NSArray *parameters = [self XMLRPCArgumentsWithExtra:media.mediaID];
+    [self.api callMethod:@"wp.deleteFile"


Hmm, couldn't find any documentation for this endpoint, was wondering if it also had some sort of success result to parse for the response.

It's actually just an alias for wp.deletePost, which returns true if successful. I guess we should check that value.

kurzee · 2017-03-10T21:52:48Z

WordPress/Classes/Services/MediaService.h

+            success:(nullable void (^)())success
+            failure:(nullable void (^)(NSError * _Nonnull error))failure;
+
+/**


multiple Media object

kurzee · 2017-03-10T22:00:37Z

WordPress/Classes/Services/MediaService.h

+ @param progress a block that will be invoked after each media item is deleted successfully
+ @param success a block that will be invoked when the media deletion finished with success
+ @param failure a block that will be invoked when there is an error.
+ */


We could be a bit more Swifty in Obj-C and also name this method deleteMedia with the different argument types from (void)deleteMedia:(nonnull Media *)media 😃

Also a : alignment issue with the declaration.

I went with deleteMultiple... to match the existing updateMultiple... method. Perhaps we could revisit this in the future (convert the whole lot to Swift? 😁)

kurzee · 2017-03-10T22:10:43Z

WordPress/Classes/Services/MediaService.m

+            // Check the media hasn't been deleted whilst we were loading.
+            if (!media || media.isDeleted) {
+                if (success) {
+                    success(nil);


Looks like this would go ahead against the _Nonnull in success:(nullable void (^)(UIImage * _Nonnull image))success.

I've adjusted this to return a new, empty UIImage. Alternative would be to change the method signature, but I think it's probably best to leave that as-is?

kurzee · 2017-03-10T22:18:41Z

WordPress/Classes/ViewRelated/Media/MediaLibraryViewController.swift

+
+    @objc private func trashTapped() {
+        let alertController = UIAlertController(title: nil,
+                                                message: NSLocalizedString("Are you sure you want to permanently delete these items?", comment: "Message prompting the user to confirm that they want to permanently delete a group of media items."), preferredStyle: .alert)


Looking at Calypso, we should also provide a singular form of this text when only selecting one item and pressing delete. Are you sure you want to permanently delete this item?

frosty · 2017-03-13T09:40:06Z

Thanks for the review, @kurzee, and for catching those edge cases! I think I've covered them all now, except for:

Looks like the Cancel bar button item stays enabled during the deletion of items, as does the trash button.

I'm not quite sure I understand. You mean in the media library view, when the HUD is visible? I just tested it here and I wasn't able to tap those items.

Care to take another look?

kurzee · 2017-03-13T22:28:36Z

For this one #6858 (comment)

I was able to reproduce, however only the "Cancel" left bar button item remained enabled, while the trash button disable, both were visible so the editing state is still enabled.

Note: I guess this is expected though since we had entered the editing state.

kurzee · 2017-03-13T22:29:34Z

I'm not quite sure I understand. You mean in the media library view, when the HUD is visible? I just tested it here and I wasn't able to tap those items.

Care to take another look?

All good on this one.

kurzee · 2017-03-13T22:48:03Z

Everything checks out, just the errors comment, which could be separate if you want or tackled with refactoring later.

Also, conflict on Podfile.lock.

when ready!

frosty · 2017-03-14T18:53:17Z

Thanks @kurzee! As discussed, staying in Edit mode is intentional and we can improve the errors later if we need to :)

frosty added 6 commits March 10, 2017 14:46

Updated MediaService and remotes to support media deletion.

a15fe5d

MediaItemViewController: Added delete UI / functionality.

3dc6dbf

Temporarily use new additions MediaPicker branch.

2e31840

Media Library: Add multiple item selection and deletion.

907d84b

Media: Refactored multiple deletion and moved code to MediaService.

4838120

Adds a test for delete media path.

0156767

frosty added the Media label Mar 10, 2017

frosty added this to the 7.2 ❄️ milestone Mar 10, 2017

frosty requested a review from kurzee March 10, 2017 15:07

SergioEstevao reviewed Mar 10, 2017

View reviewed changes

frosty mentioned this pull request Mar 10, 2017

Tweaks required for Media Library deletion in WPiOS wordpress-mobile/MediaPicker-iOS#154

Merged

kurzee suggested changes Mar 10, 2017

View reviewed changes

frosty added 7 commits March 12, 2017 14:39

Media Service: Fix comment typo.

c61f9e2

Media Library: Different prompt when deleting singular / plural items.

bf9687c

MediaService: Return empty image if media has been deleted, not nil.

ff1d6a9

MediaItemViewController: Pop if the media item gets deleted.

384874c

MediaService remotes now check response for media deletion requests.

a1ab60d

MediaLibraryPickerDataSource: Check media isn't deleted before returning

a98dae3

Media library: Update navbar item enabled state when data source changes

8215661

Merge branch 'develop' into feature/media-library-deletion

c643ab9

astralbodies modified the milestones: 7.3 ❄️, 7.2 ❄️ Mar 13, 2017

kurzee approved these changes Mar 13, 2017

View reviewed changes

frosty added 2 commits March 14, 2017 18:39

Updated MediaPicker to 0.13

ecaca8d

Merge branch 'develop' into feature/media-library-deletion

bbefa89

frosty merged commit 0297842 into develop Mar 14, 2017

frosty deleted the feature/media-library-deletion branch March 14, 2017 19:17

kurzee mentioned this pull request Mar 14, 2017

Media Library #4: Editing #6875

Merged

Conversation

frosty commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurzee commented Mar 10, 2017

Uh oh!

kurzee commented Mar 10, 2017

Uh oh!

kurzee commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurzee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frosty Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frosty commented Mar 13, 2017

Uh oh!

kurzee commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurzee commented Mar 13, 2017

Uh oh!

kurzee commented Mar 13, 2017

Uh oh!

frosty commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kurzee commented Mar 10, 2017 •

edited

Loading

frosty Mar 13, 2017 •

edited

Loading

kurzee commented Mar 13, 2017 •

edited

Loading

frosty commented Mar 14, 2017 •

edited

Loading