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

End of Year: Generate stories with dummy data + support dynamic lengths #495

Merged
merged 7 commits into from
Oct 27, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Oct 26, 2022

📘 Project: #410

iOS PR: Automattic/pocket-casts-ios#397

Creates "dummy" stories based on randomly generated podcasts and episode. This PR also adds support for dynamic story lengths.

Same as the previous tasks, the designs here are not final and are mainly placeholders.

Testing Instructions

  1. Set END_OF_YEAR_ENABLED feature flag to true in base.gradle
  2. Run the app
  3. When the prompt appears tap "View my 2022"
  4. ✅ A list of podcasts should appear ¶
  5. Wait for 2 seconds
  6. ✅ Notice that the next story starts displaying a random episode as the longest played episode
  7. Wait for 3 seconds (dynamic time set for the second story)
  8. ✅ Notice that stories progress ends
  9. Play around with previous/ next taps
  10. ✅ Notice that stories are skipped to correct positions

¶ If you don't see any podcasts, tap some podcasts on Discover or login.

Screenshots or Screencast

dummy_stories_dynamic-lengths.mp4

Checklist

N/A as only fake story views are added

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ashiagr ashiagr requested a review from a team as a code owner October 26, 2022 07:51
@ashiagr ashiagr changed the title End of Year: Create dummy stories End of Year: Create dummy stories of dynamic lengths Oct 26, 2022
@ashiagr ashiagr changed the title End of Year: Create dummy stories of dynamic lengths End of Year: Generate stories with dummy data + support dynamic lengths Oct 26, 2022
@ashiagr ashiagr mentioned this pull request Oct 26, 2022
35 tasks
Base automatically changed from task/410-add-stories-datasource to main October 26, 2022 12:13
@@ -109,6 +112,48 @@ class StoriesViewModelTest {
assertEquals(state.currentStory, story1)
}

@OptIn(ExperimentalCoroutinesApi::class)
@Test
fun `when next is invoked, then story skips to correct position`() = runTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests to check progress position seem to be flaky. They're failing in PR: #498. I'll timebox and investigate tomorrow or remove them.

Copy link
Contributor Author

@ashiagr ashiagr Oct 27, 2022

Choose a reason for hiding this comment

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

I got the tests to pass in PR: #498. Looks like I needed to add story lengths to mock stories in one of the tests. I'll monitor and address flakiness if they fail again.

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

This is coming along nicely.

The only issue I ran into is that the list items in the list of podcasts is capturing the tap events, which can make it tricky to go to the previous/next story. This can be addressed in a follow-up PR if you'd like, though. I'll hold off on merging in case you want to do that here and for the flaky tests you wanted to look into.

device-2022-10-26-163551.mp4

Comment on lines +348 to +349
@Query("SELECT * FROM podcasts LIMIT 5")
abstract fun findRandomPodcasts(): Flow<List<Podcast>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these database "random" methods just for testing purposes, or is it really supposed to return podcasts randomly?

Copy link
Contributor Author

@ashiagr ashiagr Oct 27, 2022

Choose a reason for hiding this comment

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

These are just for testing purposes and will be converted to return actual top podcasts and the longest episode. Thanks for highlighting it though!

@mchowning
Copy link
Contributor

mchowning commented Oct 26, 2022

Two additional things I noticed when testing your follow-up PR:

On a few occasions when I was playing around with landscape, the story view got messed up where it had "extra" stories, and the gaps in the top-of-the-screen indicator would change position (and not line up with the actual story changes consistently). I know that description is hard to follow, you've got to watch the video because it's pretty weird.

I reproduced this 4 times, but it was very hard to reproduce and I don't know how I reproduced it other than that it seemed to occur when I was testing landscape and switching orientations. FWIW, once I got the phone into this weird state, it would display that state in both landscape and vertical orientation.

device-2022-10-26-173009.mp4

I also noticed that the swipe down motion to close the end of year screen is too sensitive to me. Touching the screen with the slightest downward movement seems to swipe the screen away for me. Certianly not something that has to be addressed in this PR.

device-2022-10-26-172648.mp4

@ashiagr
Copy link
Contributor Author

ashiagr commented Oct 27, 2022

Thanks for testing it, @mchowning!

The only issue I ran into is that the list items in the list of podcasts is capturing the tap events, which can make it tricky to go to the previous/next story. This can be addressed in a follow-up PR if you'd like, though.

I also noticed that the swipe down motion to close the end of year screen is too sensitive to me. Touching the screen with the slightest downward movement seems to swipe the screen away for me. Certianly not something that has to be addressed in this PR.

Yep, these were not in the scope of this PR, I'll address them in future PRs.

On a few occasions when I was playing around with landscape, the story view got messed up where it had "extra" stories, and the gaps in the top-of-the-screen indicator would change position (and not line up with the actual story changes consistently). I know that description is hard to follow, you've got to watch the video because it's pretty weird.

I reproduced this 4 times, but it was very hard to reproduce and I don't know how I reproduced it other than that it seemed to occur when I was testing landscape and switching orientations. FWIW, once I got the phone into this weird state, it would display that state in both landscape and vertical orientation.

I wasn't able to reproduce it at all. Tried switching between portrait and landscape as well. Most probably we'll lock the screen to portrait just like in iOS. So I'll hold off going into this rabbit hole and I'll also discuss with you how exactly you got into this state.

EDIT: I got into this state by rapidly letting the app go into background and foreground states with the stories screen open. It is hard to reproduce and doesn't happen consistently as you noted but I'll have it fixed in a future PR.

I added these items to the main task, I'll go ahead and merge this PR.

@ashiagr ashiagr merged commit 95a5324 into main Oct 27, 2022
@ashiagr ashiagr deleted the task/410-create-dummy-stories branch October 27, 2022 07:44
@ashiagr ashiagr added this to the 7.27 ❄️ milestone Nov 24, 2022
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.

2 participants