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

Tracks for bookmarks #1282

Merged
merged 20 commits into from
Aug 18, 2023
Merged

Tracks for bookmarks #1282

merged 20 commits into from
Aug 18, 2023

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Aug 16, 2023

Description

Adding tracks for bookmarks (google sheet noting the events related to bookmarks).

For reviewing the code, you may want to go through this commit-by-commit.

Testing Instructions

In the following tests, any reference to a source property for the appropriate screen should be one of [podcast_screen, episode_details, player, files]

Verify the following on each screen with bookmarks (full screen player, podcast details, episode details, and files):

  1. Switching to the bookmarks tab on the full screen player sends the player_tab_selected event with a "tab" property of "bookmarks"
  2. From the bookmarks tab on any screen, if the upsell view is shown, tapping on the upsell prompt sends the bookmarks_upgrade_button_tapped event with the "source" property of for the relevant screen. In addition, the plus promotion_shown event is sent with a "source" of "bookmarks.
  3. From the bookmarks tab on any screen, if there are no bookmarks, tapping on the "headphone settings" button sends the bookmarks_empty_go_to_headphone_settings event with the "source" property for the screen.
  4. Creating a bookmark sends the bookmark_created event with the appropriate "source" property for the screen
  5. Updating the title of a bookmark sends the bookmark_update_titleevent with the "source" property for the screen
  6. Switching to the bookmarks tab on the screen for a podcast sends the event podcasts_screen_tab_tapped with a "value" property of "bookmarks"
  7. Switching to the bookmarks tap on the episode details screen sends the event episode_detail_tab_changed with a "value" property of "bookmarks"
    8.Tapping on the "add bookmark" option in the full screen player sends the event player_shelf_action_tapped with the "from" property of "shelf" and the "action" property of "add_bookmark"
  8. Opening the headphone controls settings sends the settings_headphone_controls_shown event
  9. Changing the next or previous headphone control actions sends the settings_headphone_controls_next_changed/settings_headphone_controls_previous_changed events (as appropriate) with a "value" property of ["add_bookmark", "next_chapter", "previous_chapter", "skip_back, skip_forward"] as appropriate
  10. With one of the headphone actions set to "Add bookmark" toggling the "Bookmark confirmation sound" setting sends the settings_headphone_controls_bookmark_confirmation_sound event with the "value" property set to whether the setting is enabled.
  11. Tapping play on a bookmark on any bookmark list sends the bookmark_play_tapped with the "source" property for the screen
  12. Changing the sort order of the bookmarks on any bookmarks list sends the bookmarks_sort_by_changed event with the "sort_order" property set to ["date_added_newest_to_oldest", "date_added_oldest_to_newest", "episode", "timestamp"] and the "source" property for the screen
  13. Deleting a bookmark sends the bookmark_deleted event with the "source" property for the screen

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • 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

@mchowning mchowning force-pushed the add/bookmarks-tracks branch from 9401a03 to 4f9ed59 Compare August 16, 2023 17:03
Base automatically changed from task/bookmark-upsell-changes to main August 17, 2023 04:28
@mchowning mchowning force-pushed the add/bookmarks-tracks branch 2 times, most recently from 81d855c to a4f2206 Compare August 17, 2023 16:33
@mchowning mchowning force-pushed the add/bookmarks-tracks branch from 0536709 to 73eafcb Compare August 17, 2023 18:43
@mchowning mchowning force-pushed the add/bookmarks-tracks branch from dba85f0 to 0dd040d Compare August 17, 2023 20:55
@mchowning mchowning marked this pull request as ready for review August 17, 2023 21:32
@mchowning mchowning requested a review from a team as a code owner August 17, 2023 21:32
Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

Awesome work, @mchowning! Thanks so much for this PR. 🙏

I only pushed two commits:

3ffe927 - Updated "Add bookmark" shelf event tracking. Earlier both shelf and overflow menu events were fired when "Add bookmark" was tapped from the overflow menu and none was fired when it was tapped from the shelf.

007f61c - Tracked bookmarks option tap from the user file details bottom sheet.

I'm approving this PR but not merging it so that you can go through my changes. Feel free to merge if you find them ok.

) : MultiSelectHelper<Bookmark>() {
override val maxToolbarIcons = 2

private val _showEditBookmarkPage = MutableSharedFlow<Boolean>()
val showEditBookmarkPage = _showEditBookmarkPage.asSharedFlow()

override var source by bookmarkManager::sourceView
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇 for using Kotlin delegated-property here. It is great to see such lesser-known language benefits being used like this.

@mchowning
Copy link
Contributor Author

Thanks for the updates @ashiagr !

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