Skip to content

Issue/11133 update page item labels - #11278

Merged
jd-alexander merged 15 commits into
feature/master-pages-offline-supportfrom
issue/11133-update-page-item-labels
Feb 12, 2020
Merged

Issue/11133 update page item labels#11278
jd-alexander merged 15 commits into
feature/master-pages-offline-supportfrom
issue/11133-update-page-item-labels

Conversation

@malinajirka

@malinajirka malinajirka commented Feb 11, 2020

Copy link
Copy Markdown
Contributor

Fixes #11133

Review instructions:

  1. Review and merge Issue/update pages item ui #11257
  2. Merge feature/master-pages-offline-support into unhandled_autosave
  3. Review this PR
  4. Wait until Offline Pages : Added Support for unpublished revision to Page List  #11228 gets merged
  5. Update the target branch with feature/master-pages-offline-support
  6. Remove the "Not ready for merge" label
  7. Merge this PR

Adds the following labels to page list items. The size of the PR seems bigger than it actually is. Most of the code was copy-pasted from post list.

Page state

  • Pending review
  • Local draft
  • Local changes
  • Private
  • Uploading page
  • Uploading draft
  • Queued page

Changes are enqueued for auto-upload

  • We'll publish the page when your device is back online.
  • We'll submit your page for review when your device is back online.
  • We'll schedule your page when your device is back online.
  • We'll publish your private page when your device is back online.
  • We'll save your draft when your device is back online

Auto-upload failed but the changes are still enqueued for auto-upload

  • We couldn't publish this page, but we'll try again later.
  • We couldn't publish this private page, but we'll try again later.
  • We couldn't schedule this page, but we'll try again later.
  • We couldn't submit this page for review, but we'll try again later.

Auto-upload failed multiple times and the changes were removed from the queue

  • We couldn't upload this media, and didn't publish the page.
  • We couldn't upload this media, and didn't publish this private page.
  • We couldn't upload this media, and didn't schedule this page.
  • We couldn't upload this media, and didn't submit this page for review.

I decided to introduce unit tests in another PR - #11281.

To test:
The business logic was copy pasted from PostListItemUiStateHelper. I'd suggest testing just a couple of scenarios. Also please make sure to review all the changes in strings.xml - I tried to replace all mentions of post with page but I might have missed something.
We'll test all the scenarios before we merge the master branch into develop.

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.

@peril-wordpress-mobile

Copy link
Copy Markdown
Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@peril-wordpress-mobile

peril-wordpress-mobile Bot commented Feb 11, 2020

Copy link
Copy Markdown

You can test the changes on this Pull Request by downloading the APK here.

@malinajirka
malinajirka changed the base branch from unhandled_autosave to issue/update-pages-item-ui February 11, 2020 09:15
@malinajirka
malinajirka changed the base branch from issue/update-pages-item-ui to unhandled_autosave February 11, 2020 10:35
uploadUiState,
pagePostStatus
)
uploadUiState.error.postError != null -> UploadUtils.getErrorMessageResIdFromPostError(

@jd-alexander jd-alexander Feb 11, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi Jirka, the error labels that are returned from here all say post instead of page I think it's because the UploadUtils.getErrorMessageResIdFromPostError only support posts at the moment and it's isPage parameter is set to false. Let me know what you think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great catch! Fixed in 0718d72

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome work :bowtie:

@jd-alexander jd-alexander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job on this @malinajirka! Most of the states and their respective labels work except for a few.

  1. These were the labels that didn't have page :

Auto-upload failed but the changes are still enqueued for auto-upload

  • We couldn't publish this page, but we'll try again later. * says post instead.
  • We couldn't publish this private page, but we'll try again later. * says post instead
  • We couldn't schedule this page, but we'll try again later. * says post instead.
  • We couldn't submit this page for review, but we'll try again later.* says post instead.
  1. I wasn't able to get these labels showing due to the constant issues :

Auto-upload failed multiple times and the changes were removed from the queue

  • We couldn't upload this media, and didn't publish the page.
  • We couldn't upload this media, and didn't publish this private page.
  • We couldn't upload this media, and didn't schedule this page.
  • We couldn't upload this media, and didn't submit this page for review.

Doing some of the tests proved difficult, due to the constant NullPointerExceptions with the PostModel which we have constantly encountered while doing this project so we will have to do another round of tests once we have resolved this.

  1. One of the exemptions I got were
E/AndroidRuntime: FATAL EXCEPTION: DefaultDispatcher-worker-12
    Process: org.wordpress.android.beta, PID: 14952
    java.lang.IllegalStateException: postModel must not be null
        at org.wordpress.android.viewmodel.pages.PageListViewModel.createItemUiStateData(PageListViewModel.kt:364)
        at org.wordpress.android.viewmodel.pages.PageListViewModel.prepareTrashedPages(PageListViewModel.kt:310)
        at org.wordpress.android.viewmodel.pages.PageListViewModel.access$prepareTrashedPages(PageListViewModel.kt:51)
        at org.wordpress.android.viewmodel.pages.PageListViewModel$loadPagesAsync$1.invokeSuspend(PageListViewModel.kt:178)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:561)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:727)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:667)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:655)


  1. And another one was
    java.lang.IllegalStateException: page must not be null
        at org.wordpress.android.viewmodel.pages.PagesViewModel.onItemTapped(PagesViewModel.kt:472)
        at org.wordpress.android.viewmodel.pages.PageListViewModel.onItemTapped(PageListViewModel.kt:140)
        at org.wordpress.android.ui.pages.PageListFragment$setPages$2.invoke(PageListFragment.kt:109)
        at org.wordpress.android.ui.pages.PageListFragment$setPages$2.invoke(PageListFragment.kt:27)
        at org.wordpress.android.ui.pages.PageItemViewHolder$PageViewHolder$onBind$$inlined$let$lambda$1.onClick(PageItemViewHolder.kt:89)
        at android.view.View.performClick(View.java:7125)

When I attempted to open a DraftPage that had several media items attached in offline mode.

Two other issues I noticed that weren't related to this PR but are up for discussion.

  1. Sometimes there is a delay before all pages are loaded. There will be a few PageItems within the list and then the rest appear.

  2. I created a new page and when I set it's status to "Private" the action changes from "Publish" to "Update" even though the page was never published. I am not sure if this is intended since the status of the page is indeed being changed so let me know.

Comment thread WordPress/src/main/java/org/wordpress/android/ui/uploads/UploadUtils.java Outdated
Comment thread WordPress/src/main/java/org/wordpress/android/ui/uploads/UploadUtils.java Outdated
Comment thread WordPress/src/main/java/org/wordpress/android/ui/uploads/UploadUtils.java Outdated
@malinajirka

Copy link
Copy Markdown
Contributor Author

Thanks for the great review!!

  1. These were the labels that didn't have page :

Fixed in 0718d72

  1. I wasn't able to get these labels showing due to the constant issues :

I tested these scenarios and they look okay.

  1. Set emulator's network type to EDGE
  2. Create a draft and publish it
  3. Turn on the airplane mode
  4. Wait until the upload fails
  5. Turn off the airplade mode and turn it on again
  6. Wait until the upload fails
  7. Repeat 5 and 6 three times and the message appears
  1. and 4.

We should fix both these issues in #11271

Sometimes there is a delay before all pages are loaded. There will be a few PageItems within the list and then the rest appear.

I'm not sure it's the issue you are talking about. But this happens to me when the pages are loaded from local database before they are fetched from the remote. Or is it happening to you even when the pages should already be in the local database? If so, can you reproduce the steps?

I created a new page and when I set it's status to "Private" the action changes from "Publish" to "Update" even though the page was never published. I am not sure if this is intended since the status of the page is indeed being changed so let me know.

This is an expected state, having said that I agree it's not optimal and we'll hopefully improve it in one of the upcoming projects.

@jd-alexander jd-alexander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the changes @malinajirka I ran another set of test on the changes and all the labels are behaving as expected!

There was a minor conflict when I changed the base branch so I resolved it & formatted the code so it could be merged!

I noticed an issue I think is unrelated but sharing for your perusal.

  1. Create a page and publish.
  2. While it’s being published turn airplane mode on.
  3. Turn it off and back on again in quick succession. (Sometimes, you don’t even need this step for the below behavior to begin.)
  4. There’s a flicker of the UI and the notification.
Flicker Issue
https://drive.google.com/file/d/1KWf9_zPJjMV6M66L-jbLBeoth_R7QfRR/view?usp=sharing

Let me know your thoughts on it after reproduction. I am going to create an issue once necessary after you review. Thanks


In response to your responses 😄

I tested these scenarios and they look okay.

Set emulator's network type to EDGE
Create a draft and publish it
Turn on the airplane mode
Wait until the upload fails
Turn off the airplade mode and turn it on again
Wait until the upload fails
Repeat 5 and 6 three times and the message appears
and 4.

Thanks! I did this and got things working appropriately!

We should fix both these issues in #11271

Indeed!

I'm not sure it's the issue you are talking about. But this happens to me when the pages are loaded from local database before they are fetched from the remote. Or is it happening to you even when the pages should already be in the local database? If so, can you reproduce the steps?

Oh yes! That's exactly when it happens! Thanks for clearing this up!

This is an expected state, having said that I agree it's not optimal and we'll hopefully improve it in one of the upcoming projects.

Understood!

@jd-alexander
jd-alexander changed the base branch from unhandled_autosave to feature/master-pages-offline-support February 12, 2020 20:59
@jd-alexander
jd-alexander merged commit adc298c into feature/master-pages-offline-support Feb 12, 2020
@jd-alexander
jd-alexander deleted the issue/11133-update-page-item-labels branch February 12, 2020 23:11
@malinajirka

malinajirka commented Feb 13, 2020

Copy link
Copy Markdown
Contributor Author

Thanks you for testing this again @jd-alexander !!

Let me know your thoughts on it after reproduction. I am going to create an issue once necessary after you review. Thanks

I wasn't able to reproduce it. Could you please fill a bug report and ideally include

  • logs
  • does this state ever end or is the app in some kind of a loop?
  • gif/video which includes even the steps how you got into that state
  • if it happens even on develop (in post list), add it to groundskeeping please

Thanks!

@malinajirka malinajirka modified the milestones: 14.3, 14.6 Mar 23, 2020
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.

3 participants