[HOLD for Payment 2024-09-06][$250] mWeb - Video - In welcome video, all playback speed options is not shown in normal mode

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.18
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Login as a Gmail account
3. In the Welcome Expensify video, tap 3 dots
4. Select playback speed
5. Note the number of playback speed options
6. Go to full screen mode
7. Tap 3 dots - playback speed
8. Note the number of playback speed options

Expected Result:

In the welcome video, all playback speed options shown is full screen mode must be shown in normal mode.

Actual Result:

In the welcome video, all playback speed options shown is full screen mode is not shown in normal mode.

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6568733_1723364226029.Screenrecorder-2024-08-11-13-35-19-491_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014d75aeb575f9f1ea
• Upwork Job ID: 1823445367470479721
• Last Price Increase: 2024-08-13
• Automatic offers:
• ikevin127 | Reviewer | 103548572
• neonbhai | Contributor | 103548574

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

We think that this bug might be related to #vip-vsb

[IuliiaHerets]

@slafortune FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[neonbhai]

Proposal

Please re-state the problem that we are trying to solve in this issue.

In welcome video, all playback speed options is not shown in normal mode

What is the root cause of that problem?

We are missing playback speeds that are offered by the native players

What changes do you think we should make in order to solve the problem?

We should add the missing playback speeds of 0.75 & 1.25 here

Result:

Video

Screen.Recording.2024-08-12.at.5.45.49.AM.mov

[daledah]

Edited by proposal-police: This proposal was edited at 2024-08-12 01:07:01 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

In the welcome video, all playback speed options shown is full screen mode is not shown in normal mode.

What is the root cause of that problem?

The playback speed options shown in full screen mode contain [0.25, 0.5, 0.75, Normal, 1.25, 1.5, .175, 2] but in normal mode, it is just [0.25, 0.5, 1, 1.5, 2]:

App/src/CONST.ts

Line 4164 in 0341c3b

PLAYBACK_SPEEDS: [0.25, 0.5, 1, 1.5, 2],

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

Line 58 in 0341c3b

subMenuItems: CONST.VIDEO_PLAYER.PLAYBACK_SPEEDS.map((speed) => ({

What changes do you think we should make in order to solve the problem?

1. We need to add the missing playback speed value in normal mode:
   

   App/src/CONST.ts

   Line 4164 in 0341c3b

   PLAYBACK_SPEEDS: [0.25, 0.5, 1, 1.5, 2],

   
   will be:

        PLAYBACK_SPEEDS: [0.25, 0.5, 0.75, 1, 1.25, 1.5, 1.75, 2],

2. Update the initial playback speed:
   

   App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

   Line 18 in 0341c3b

   const [currentPlaybackSpeed, setCurrentPlaybackSpeed] = useState<PlaybackSpeed>(CONST.VIDEO_PLAYER.PLAYBACK_SPEEDS[2]);

    const [currentPlaybackSpeed, setCurrentPlaybackSpeed] = useState<PlaybackSpeed>(CONST.VIDEO_PLAYER.PLAYBACK_SPEEDS[3]);

3. Optionally, with splayback speed value is 1, we can display Normal instead of 1 so it will be the same as what we did in fullscreen mode:
   

   App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx

   Line 60 in 0341c3b

   text: speed.toString(),

                text: speed !== 1 ?  speed.toString() : "Normal",

What alternative solutions did you explore? (Optional)

[daledah]

Proposal updated

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014d75aeb575f9f1ea

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 (External)

[slafortune]

Adding BZ again - I'll be out until 8/21

[melvin-bot]

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[greg-schroeder]

Awaiting proposal review by @ikevin127

[ikevin127]

@daledah Thank you for the proposal. While I appreciate the more detailed proposal, unfortunately it came after the first one which I think was detailed enough since this issue has a simple root cause / solution.

ℹ️ Keep in mind the contributor guidelines (6.) which state:

Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

Otherwise you could get a ⚠️ warning, which after 2 (same month) or 3 (per year) you risk being removed from the contributor program.

[ikevin127]

@neonbhai's proposal looks good to me even though it's not extremely detailed, since this issue has a simple root cause / solution. The root cause was pointed out correctly and the proposed solution fixes the issue, even though it does not mention all the details, it mentions enough to be acceptable.

Notes for PR (if assigned):

1. Make sure to adjust the types as well based on the new added values.
2. Make sure to adjust the initial state variable currentPlaybackSpeed to match the Normal speed, given the new values.
3. To fully fulfil the Expected result, please have the value 1 show the label Normal in order to match the native player.
   Note: The label Normal should be translated (en/es) and placed in translations at videoPlayer.playbackSpeedNormal (or similar).

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[daledah]

@ikevin127 Thanks for your feedback. I agree with your final decision, and the details can be updated during the PR stage if @neonbhai is assigned. However, I’d like to clarify why I initially posted a proposal in this case:

• The first proposal missed the 1.75 value.
• I included code changes to update the default volume state based on the new PLAYBACK_SPEEDS value.
• I suggested displaying 1 as Normal.

[daledah]

Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

@ikevin127 Also, the guideline you referred to is quite subjective (as you also say "I think was detailed enough"), different people could have different opinions on whether the difference between proposals is substantial or not. I only posted my proposal because I believe my proposal is substantially different.

Otherwise you could get a ⚠️ warning, which after 2 (same month) or 3 (per year) you risk being removed from the contributor program.

I don't think it'd be right to penalize contributors because of a subjective standard, especially when their intention is to contribute better solutions. This doesn't harm Expensify at all and will lead to a more complete solution overall. If we penalize contributors for this reason people'd be discouraged from contributing even though they see a better solution, because they're afraid the C+'s personal opinion could lead to warnings for contributors.

At last, I agree with your proposal selection but I'd appreciate if you consider my 2 points above on not penalizing contributors for this reason. Thanks!

[ikevin127]

@daledah Don't worry - you will not get a warning in this case, I was just mentioning to be mindful of that contributor guideline in the future as it might lead to warnings.

Of course it's subjective, there are cases in which a more detailed RCA and solution are required - I don't consider this one to be that case.

[daledah]

@ikevin127 Sure thank you!

[melvin-bot]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @neonbhai 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

@slafortune, @greg-schroeder, @techievivek, @ikevin127, @neonbhai Huh... This is 4 days overdue. Who can take care of this?

[ikevin127]

Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻

Currently awaiting for @neonbhai to give a timeline on the PR.

Please keep in mind the PR notes from #47205 (comment):

1. Make sure to adjust the types as well based on the new added values.
2. Make sure to adjust the initial state variable currentPlaybackSpeed to match the Normal speed, given the new values.
3. To fully fulfil the Expected result, please have the value 1 show the label Normal in order to match the native player.
   Note: The label Normal should be translated (en/es) and placed in translations at videoPlayer.playbackSpeedNormal (or similar).

[neonbhai]

Thanks for the note @ikevin127

The PR is here

[ikevin127]

⚠️ We just got confirmation on Slack that the Deploy Checklist: New Expensify 2024-08-26 which includes the PR of this issue was only deployed to production today in Deploy Checklist: New Expensify 2024-08-28. More context on why this happened can be found in this Slack thread and this Slack comment.

Given the context above, this issue should be on [HOLD for Payment 2024-09-06] according to 4 days' ago production deploy from Deploy Checklist: New Expensify 2024-08-28.

cc @slafortune @techievivek

[slafortune]

@ikevin127 Thanks! - Can you please complete the checklist -
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ikevin127 ] The PR that introduced the bug has been identified. Link to the PR:
• [@ikevin127 ] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@ikevin127 ] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@ikevin127 ] Determine if we should create a regression test for this bug.
• [@ikevin127 ] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@slafortune ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Video player #30829
• [@ikevin127] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/30829/files#r1744646106
• [@ikevin127] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Open Expensify app and login.
2. Open a chat and send a video attachment.
3. Click on Expand (to make video fullscreen).
4. Click on three dots -> Playback speed.
5. Verify that the 1x speed is labeled Normal.
6. Verify that all playback speed options (0.25, 0.5, 0.75, Normal, 1.25, 1.5, 1.75, 2) can be observed.

Do we agree 👍 or 👎.

[slafortune]

Regression Test - https://github.com/Expensify/Expensify/issues/426444
@ikevin127 @neonbhai Paid ✅ ✅