-
-
Notifications
You must be signed in to change notification settings - Fork 749
Year in Review Reading List with A/B Test #6045
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
Conversation
…es the strings xml - code fixes
… save that list and code to update ui for that list - ui/code fixes and adds strings resources
- updates YearInReviewSurvey.kt to YearInReviewDialog.kt - code fixes/ui fixes
…e "new" indicator once the user opens the new list - code fixes
# Conflicts: # app/src/main/java/org/wikipedia/yearinreview/YearInReviewScreenDeck.kt # app/src/main/res/values/strings_no_translate.xml
cooltey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue found:
After the reading list is created, we will still see the "Reading list created" after going back to the main reading lists screen. Not sure if this is something needed to confirm with PM.
app/src/main/java/org/wikipedia/readinglist/ReadingListFragmentViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/wikipedia/readinglist/ReadingListFragmentViewModel.kt
Outdated
Show resolved
Hide resolved
| private fun hideNewIndicatorForRecentSavedList() { | ||
| if (recentPreviewSavedReadingList != null) { | ||
| val pos = displayedLists.indexOfFirst { it is ReadingList && it.id == recentPreviewSavedReadingList?.id } | ||
| if (pos != -1) { | ||
| adapter.notifyItemChanged(pos) | ||
| } | ||
| recentPreviewSavedReadingList = null | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure but do we really need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the design
app/src/main/java/org/wikipedia/yearinreview/YearInReviewDialog.kt
Outdated
Show resolved
Hide resolved
| const val MIN_SLIDES_BEFORE_SURVEY = 2 | ||
| const val MIN_SLIDES_FOR_CREATING_YIR_READING_LIST = 1 | ||
| const val MIN_ARTICLES_FOR_CREATING_YIR_READING_LIST = 5 | ||
| const val CUT_OFF_DATE_FOR_SHOWING_YIR_READING_LIST_DIALOG = "2026-03-31T23:59:59Z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this in RemoteConfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean not adding these values here in YearInReviewModel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are putting all the dates in the feed/configuration endpoint, maybe we can think about putting this date into the remote configuration, too.
But we can do this later if we decide to move this to the remoteConfig.
I am not sure I follow. could you share a quick recording or screenshot so I can see what you mean? |
…ogs and reading list - adds loading and error UI for year in review reading list - code fixes
* - adds logic/enum state prefs to show yir reading list survey * - code fixes * - code fix
| const val MIN_SLIDES_BEFORE_SURVEY = 2 | ||
| const val MIN_SLIDES_FOR_CREATING_YIR_READING_LIST = 1 | ||
| const val MIN_ARTICLES_FOR_CREATING_YIR_READING_LIST = 5 | ||
| const val CUT_OFF_DATE_FOR_SHOWING_YIR_READING_LIST_DIALOG = "2026-03-31T23:59:59Z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are putting all the dates in the feed/configuration endpoint, maybe we can think about putting this date into the remote configuration, too.
But we can do this later if we decide to move this to the remoteConfig.
| import java.time.ZoneOffset | ||
|
|
||
| object YearInReviewDialog { | ||
| private val userGroup = if (Prefs.appInstallId.hashCode() % 2 == 0) "A" else "B" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am kind of concerned about this:
Will the same group of users fall into the same user group if we keep using the app install ID as the seed?
Similar to this one
Lines 185 to 187 in 3020fa9
| private fun getUserGroup(): String { | |
| return if (Prefs.appInstallId.hashCode() % 2 == 0) "A" else "B" | |
| } |
I think there's a proper way of assigning the users to a group by following the logic in ABTest.assignGroup().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this logic will always keep the user in the same group because the appInstallId is always same for the app if (Prefs.appInstallId.hashCode() % 2 == 0) "A" else "B". The ABTest.assignGroup() logic has a preference to store the test group, but if we do something similar to assignGroup() then we would have to add another preference to save this user group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can reuse the ABTest class to create an AB test group, which makes more sense in this case.


What does this do?
Phabricator:
https://phabricator.wikimedia.org/T407656