Page MenuHomePhabricator
Create Task
Maniphest T320728

Mentee overview(vue): Empty string in "Maximum"/"Minimum" filter options is not persisted
Closed, ResolvedPublicBUG REPORT
Actions

Description

Steps to replicate the issue (include links if applicable):

  • As a mentor, open Special:MentorDashboard
  • Click "Filters" and in the dialog, put an empty string in place of the "Maximum" or "Minimum" field (like at F35566354)
  • Click "Update"
  • Reload the page
  • Click "Filters" and examine the state of the filters

What happens?:

The filters were reset to 1/500 (minimum/maximum), aka the default state.

What should have happened instead?:

The empty string (=filter is not used) should be persisted for next load, using the presets preference.

Other information (browser name/version, screenshots, etc.):
Tested in enwiki with the Vue deployment patch applied.

Details

SubjectRepoBranchLines +/-
Mentee filters: always use mw.user.options values to initialise the mentees storemediawiki/extensions/GrowthExperimentswmf/1.40.0-wmf.5+17 -19
Mentee filters: always use mw.user.options values to initialise the mentees storemediawiki/extensions/GrowthExperimentsmaster+17 -19
Customize query in gerrit

Related Objects
Search...

Event Timeline

Urbanecm_WMF created this task.Oct 13 2022, 2:01 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 13 2022, 2:01 PM
Urbanecm_WMF moved this task from Incoming to Ready for Development on the Growth-Team (Sprint 0 (Growth Team)) board.Oct 13 2022, 2:01 PM
Urbanecm_WMF added a parent task: T300532: Migration of mentee overview to Vue.
Urbanecm_WMF updated the task description. (Show Details)
Sgs claimed this task.Oct 13 2022, 2:12 PM
Maintenance_bot added a project: GrowthExperiments-Mentorship.Oct 13 2022, 2:30 PM
Sgs mentioned this in T320722: Deployment training request for Sergio Gimeno.Oct 13 2022, 2:30 PM
Maintenance_bot moved this task from New Tasks to In Progress on the GrowthExperiments-Mentorship board.Oct 13 2022, 3:15 PM
Urbanecm_WMF mentioned this in T300532: Migration of mentee overview to Vue.Oct 14 2022, 10:16 AM
gerritbot added a comment.Oct 14 2022, 11:52 AM

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

[mediawiki/extensions/GrowthExperiments@master] Mentee filters: always use mw.user.options values to initialise the mentees store

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

gerritbot added a project: Patch-For-Review.Oct 14 2022, 11:52 AM
Sgs added a comment.Oct 14 2022, 12:13 PM

@Urbanecm_WMF Thanks for filing this. I have pushed a patch that should fix the issue. I have one question, see below:

Right now, in the Vue dashboard, the user preferences save call ( user-preferences.js#saveOption ) is not setting the mw.user.options when doing the Api request as opposed of the prior (MenteeOverviewPresets.js#setPreset). Should we do the same here for consistency? I'm more on the side the setting should happen just if the request is successful.

Since the Vuex stores are for reactive data but we're manually setting mw.user.options I wonder if we should strip this out of stores/modules and have them somewhere else just as convenience methods. (ie: MenteeOverviewApi.js).

Sgs moved this task from Ready for Development to Blocked / Needs work on the Growth-Team (Sprint 0 (Growth Team)) board.Oct 14 2022, 12:16 PM
Urbanecm_WMF added a subscriber: kostajh.Oct 14 2022, 1:52 PM

Thanks for the quick fix @Sgs! Merged.

Ad calling mw.user.options.set, good question. That call was added in d665b46cb by @kostajh, with "call mw.user.options.set so the setPreset change takes immediate effect" as the comment. I'm not sure why would that be needed, AFAICS, mw.user.options.get is only called when the non-Vue mentee overview module is inicialized, and then it keeps its state internally. I don't think calling or not calling mw.user.options.set would make any difference.

Urbanecm_WMF triaged this task as Medium priority.Oct 14 2022, 1:54 PM
gerritbot added a comment.Oct 14 2022, 2:00 PM

Change 842751 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Mentee filters: always use mw.user.options values to initialise the mentees store

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

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.6; 2022-10-17).Oct 14 2022, 2:00 PM
Maintenance_bot removed a project: Patch-For-Review.Oct 14 2022, 2:30 PM
gerritbot added a comment.Oct 14 2022, 10:49 PM

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

[mediawiki/extensions/GrowthExperiments@wmf/1.40.0-wmf.5] Mentee filters: always use mw.user.options values to initialise the mentees store

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

gerritbot added a project: Patch-For-Review.Oct 14 2022, 10:49 PM
Urbanecm_WMF added a comment.Oct 14 2022, 10:49 PM

I think the fix should be backported next week (followed with enabling Vue everywhere). That way, we should be able to fix any additional potential regressions, should they be noticed only after rolling Vue out. Uploaded a patch suggesting that.

gerritbot added a comment.Oct 17 2022, 7:50 AM

Change 842897 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@wmf/1.40.0-wmf.5] Mentee filters: always use mw.user.options values to initialise the mentees store

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

Stashbot added a comment.Oct 17 2022, 7:50 AM

Mentioned in SAL (#wikimedia-operations) [2022-10-17T07:50:22Z] <urbanecm@deploy1002> Started scap: Backport for [[gerrit:842897|Mentee filters: always use mw.user.options values to initialise the mentees store (T320728)]]

Stashbot added a comment.Oct 17 2022, 7:50 AM

Mentioned in SAL (#wikimedia-operations) [2022-10-17T07:50:48Z] <urbanecm@deploy1002> urbanecm and urbanecm: Backport for [[gerrit:842897|Mentee filters: always use mw.user.options values to initialise the mentees store (T320728)]] synced to the testservers: mwdebug2001.codfw.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1001.eqiad.wmnet

Stashbot added a comment.Oct 17 2022, 7:57 AM

Mentioned in SAL (#wikimedia-operations) [2022-10-17T07:57:44Z] <urbanecm@deploy1002> Finished scap: Backport for [[gerrit:842897|Mentee filters: always use mw.user.options values to initialise the mentees store (T320728)]] (duration: 07m 22s)

ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.5; 2022-10-10); removed MW-1.40-notes (1.40.0-wmf.6; 2022-10-17).Oct 17 2022, 8:00 AM
Maintenance_bot removed a project: Patch-For-Review.Oct 17 2022, 8:30 AM
kostajh added a comment.Oct 17 2022, 9:22 AM
In T320728#8316999, @Urbanecm_WMF wrote:

Thanks for the quick fix @Sgs! Merged.

Ad calling mw.user.options.set, good question. That call was added in d665b46cb by @kostajh, with "call mw.user.options.set so the setPreset change takes immediate effect" as the comment. I'm not sure why would that be needed, AFAICS, mw.user.options.get is only called when the non-Vue mentee overview module is inicialized, and then it keeps its state internally. I don't think calling or not calling mw.user.options.set would make any difference.

I can't remember now, but I think it may have been needed for the QUnit test.

kostajh moved this task from Blocked / Needs work to QA on the Growth-Team (Sprint 0 (Growth Team)) board.Oct 18 2022, 3:11 PM
Etonkovidova closed this task as Resolved.Oct 25 2022, 11:11 PM
Etonkovidova subscribed.

Checked in testwiki wmf.7- works as expected.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL