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

IuliiaHerets commented 2 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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:

Go to https://staging.new.expensify.com/home
Login as a Gmail account
In the Welcome Expensify video, tap 3 dots
Select playback speed
Note the number of playback speed options
Go to full screen mode
Tap 3 dots - playback speed
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
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/7b786d01-cf13-4576-b4f9-d480de7d4569

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[bot] commented 2 months ago

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 commented 2 months ago

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

IuliiaHerets commented 2 months ago

@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 commented 2 months ago

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

https://github.com/user-attachments/assets/f46003bd-8c38-4243-a91c-3f637e9f717e

daledah commented 2 months ago

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]: https://github.com/Expensify/App/blob/0341c3b4e43c6b483f64c9c79b85eeebfdc6faf1/src/CONST.ts#L4164 https://github.com/Expensify/App/blob/0341c3b4e43c6b483f64c9c79b85eeebfdc6faf1/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx#L58

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

We need to add the missing playback speed value in normal mode: https://github.com/Expensify/App/blob/0341c3b4e43c6b483f64c9c79b85eeebfdc6faf1/src/CONST.ts#L4164 will be:
```
    PLAYBACK_SPEEDS: [0.25, 0.5, 0.75, 1, 1.25, 1.5, 1.75, 2],
```
Update the initial playback speed: https://github.com/Expensify/App/blob/0341c3b4e43c6b483f64c9c79b85eeebfdc6faf1/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx#L18
```
const [currentPlaybackSpeed, setCurrentPlaybackSpeed] = useState<PlaybackSpeed>(CONST.VIDEO_PLAYER.PLAYBACK_SPEEDS[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: https://github.com/Expensify/App/blob/0341c3b4e43c6b483f64c9c79b85eeebfdc6faf1/src/components/VideoPlayerContexts/VideoPopoverMenuContext.tsx#L60
```
            text: speed !== 1 ?  speed.toString() : "Normal",
```
What alternative solutions did you explore? (Optional)

daledah commented 2 months ago

Proposal updated

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

slafortune commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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 commented 2 months ago

Awaiting proposal review by @ikevin127

ikevin127 commented 2 months ago

♻️ Reviewing proposals today.

ikevin127 commented 2 months ago

@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 commented 2 months ago

@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):

Make sure to adjust the types as well based on the new added values.
Make sure to adjust the initial state variable currentPlaybackSpeed to match the Normal speed, given the new values.
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[bot] commented 2 months ago

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

daledah commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@ikevin127 Sure thank you!

melvin-bot[bot] commented 2 months ago

📣 @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[bot] commented 2 months ago

📣 @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[bot] commented 2 months ago

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

ikevin127 commented 2 months ago

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 https://github.com/Expensify/App/issues/47205#issuecomment-2290157181:

Make sure to adjust the types as well based on the new added values.
Make sure to adjust the initial state variable currentPlaybackSpeed to match the Normal speed, given the new values.
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 commented 2 months ago

Thanks for the note @ikevin127

The PR is here

ikevin127 commented 2 months ago

⚠️ 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 commented 1 month ago

@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.
[x] [@slafortune ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

ikevin127 commented 1 month ago

[x] [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/30829
[x] [@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
[x] [@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.
[x] [@ikevin127] Determine if we should create a regression test for this bug.
[x] [@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

Open Expensify app and login.
Open a chat and send a video attachment.
Click on Expand (to make video fullscreen).
Click on three dots -> Playback speed.
Verify that the 1x speed is labeled Normal.
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 commented 1 month ago

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

Expensify / App