Page MenuHomePhabricator

[wmf.28 - testwiki] MentorDashboard - visible star state is not persistent
Closed, ResolvedPublicBUG REPORT

Description

It's only a display issue - the starred status is, in fact, correctly stored.

Steps to replicate the issue :

  • As a mentor go to Special:MentorDashboard (testwiki wmf.28)
  • Mark any of your mentees as starred (click on a star at a mentee's user name) - the star becomes black. A mentee is starred.

What happens?:

  • Perform any sorting on the columns - the star will become non-black.
  • Open Filters and select the option "Only show starred mentees" - the mentee that you just starred will be display, but the star is displayed as non-black.

What should have happened instead?:

  • Once a mentee is starred (the star is clicked), the status of the star should stay as marked. It should not change with any sorting or filter options.

Other information (browser name/version, screenshots, etc.):

  • it could be confusing to a user who doesn't see whether the starred status is saved.
  • the animated gif below shows
    • two mentees were starred
    • Filters' option - "Only show starred mentees" was applied
    • two starred mentees are shown with the stars displayed as non-black.

mentor_dashboard_stars.gif (734×701 px, 199 KB)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

@Sgs we need to fix before we rollout the Vue mentor dashboard, right?

Etonkovidova renamed this task from [wmf.28 - testwiki] MentorDashboard - visible star state is not peristent to [wmf.28 - testwiki] MentorDashboard - visible star state is not persistent .Sep 7 2022, 7:47 PM

Change 830952 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] MenteeOverviewAPI: export a single instance of the API

https://gerrit.wikimedia.org/r/830952

@Sgs we need to fix before we rollout the Vue mentor dashboard, right?

I would prefer so.

I think the root of the issue is at a “design” level. MenteeOverviewApi has state since it holds the starredMentees property with an array of IDs of the starred mentees. Star/unstar methods update such array after every update. In the non-Vue version of the dashboard the MenteeOverviewApi is instantiated only one time in MenteeOverview.js so there's always one single collection of starred mentees in play. In the Vue version I (naively) instantiated two times the api, one in the mentees store (mentees.js) and in the MenteeOverview component. This leads to having two different arrays of starredMentees and the filter actions that are triggered after starring/unstarring a mentee end up using different and not updated collections.

My understanding is that having state in the API is fine when it is instantiated per page but if we want to be able to instantiate in different components or stores having state there is not a great deal. A hotfix could be to use a singleton for it (see gerrit 830952 ). Ideally I would advocate for eliminating the MenteeOverviewAPI in favor of a simplified http client and place the "business logic" (if any) of the requests directly in the mentees store.

Alternatively we could leave the MenteeOverviewAPI as a "factory" and instantiate it only from the mentees store but my perception is that it's not useful and can lead into the same bug again. Feedback welcome @Urbanecm_WMF

Sgs changed the task status from Open to In Progress.Sep 8 2022, 11:07 PM
Sgs moved this task from In Progress to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.

Change 830952 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] MenteeOverviewAPI: export a single instance of the API

https://gerrit.wikimedia.org/r/830952

Urbanecm_WMF changed the task status from In Progress to Open.Sep 9 2022, 4:41 PM

Checked in betalabs and testwiki wmf.1 - works as expected.