-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Due for payment 2025-02-10] [$250] Remove onboarding videos from all the onboarding flows #55642
Comments
Job added to Upwork: https://www.upwork.com/jobs/~021882162574801556686 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @c3024 ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Remove onboarding videos from all the onboarding flows What is the root cause of that problem?Feature removal request What changes do you think we should make in order to solve the problem?Remove the videos from the message creation flow at the following places: Lines 104 to 107 in 9340e36
Lines 157 to 162 in 9340e36
Lines 5178 to 5182 in 9340e36
Lines 4992 to 4996 in 9340e36
Lines 5131 to 5135 in 9340e36
Optional: I think we need to remove the What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?N/A What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.Remove onboarding videos from all the onboarding flows What is the root cause of that problem?Improvement What changes do you think we should make in order to solve the problem?
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?Not needed What alternative solutions did you explore? (Optional) |
|
🚨 Edited by proposal-police: This proposal was edited at 2024-09-28 10:38:21 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.The goal is to remove all onboarding videos from the onboarding flows. What is the root cause of that problem?This request is based on a feature removal decision. What changes do you think we should make in order to solve the problem?
Lines 263 to 264 in 9340e36
Lines 106 to 112 in 9340e36
Lines 160 to 166 in 9340e36
Lines 4994 to 5000 in 9340e36
Lines 5133 to 5139 in 9340e36
Lines 5180 to 5186 in 9340e36
App/src/libs/actions/Report.ts Lines 3660 to 3670 in 9340e36
App/src/libs/actions/Report.ts Lines 4035 to 4063 in 9340e36
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?What alternative solutions did you explore? (Optional)None at this time. Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
|
Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@c3024 I think those are implementation details, my proposal did cover the thing needed to complete the task in the OP, so i guess the selection is not fair based only code clean up :/ Actually my point is that according to the contributors doc:
So my proposal yields the same result even if we do not do the cleanup as suggested by other contributor. So results is the same which will be achieved so other proposal is not at all different, and i honestly think that the extra things suggested are a code cleanup which can be done during PR phase c.c. @Gonals |
@c3024 so are we ready to move to the next phase? Can you outline next steps and get this one moving please? |
I chose @FitseTLT’s proposal because it is complete and addresses all the changes required to remove the video-related logic. It is true that your proposal also works and yields the same result. I feel this is a simple issue, and all the required changes can be identified and included by the contributor in the initial proposal itself. Moreover, there is a possibility that code cleanup might be missed, especially since simply removing the constants from the file also works. So, I’d like to stick with my decision to select @FitseTLT’s proposal for moving forward. The next step is for @Gonals to choose one of the external contributors to work on the PR. Thanks! |
Thanks for agreeing on this.
I thought counter intuitively 😅 that this is a simple issue and would only need the crux part of the changes required and not the cleanup .
Thanks for replying, though i still think that my proposal should be choosen here as it's complete and works according to OP and shouldn't be ignored because i didn't include code cleanup 😢 . @Gonals can you please review my POV too before making a proposal selection, thanks! |
📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
I agree with your logic. Assigning to @FitseTLT |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.93-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2025-02-10. 🎊 For reference, here are some details about the assignees on this issue:
|
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
I'm still seeing the onboarding video on my onboarding flows on staging and production? Should I be seeing it still? 2025-02-04_11-45-59.mp4 |
I think this is a gif @danielrvidal It has already been mentioned here that it needs a BE fix. |
Got it, so is that on @Gonals to help update from our side? |
Issue is ready for payment but no BZ is assigned. @trjExpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks! |
@Gonals don't suppose you can confirm removal of that GIF from the backend? |
Yep. I haven't gotten to it yet. I'll create a separate issue for it, so we don't have to hold this one for it. |
Okay, let's track that in #convert though (CC: @danielrvidal). Added it to the project. |
@trjExpensify, I take we want to remove the GIF, but geep the rest of the message, right? |
Yes, I believe that was @danielrvidal's intent. |
We have onboarding videos in the welcome message that comes from Concierge for most flows and from the guide/sales in the #admins room for users who created a workspace or are an admin.
We did an a/b test and found the videos are now less effective at generating usage than not having the videos. Thus, we would like to remove them from all cases.
We show different variations of the video based on their introSelected NVP. We need to remove it from all cases. The NVP looks like:
And here you can see the video for the employee (the NVP above) case here:
![Image](https://private-user-images.githubusercontent.com/2364487/405778850-7aa73124-7a04-4bc5-a3b6-72941681e92b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzNDUxNTksIm5iZiI6MTczOTM0NDg1OSwicGF0aCI6Ii8yMzY0NDg3LzQwNTc3ODg1MC03YWE3MzEyNC03YTA0LTRiYzUtYTNiNi03Mjk0MTY4MWU5MmIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxMiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTJUMDcyMDU5WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ODI5OWU0ZGVhNzM4ODdiN2NiNWY0ZjI5ZGE4Mzg4YjdhYmQ0NDNmYzcxZDU2MzQ3NmVjNTNlZDMyZTMxNmY0NyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.zKcmRUJqX-JyLP7O2Rr2eq7b9zG0_k3N_82gOLgQRbQ)
Please let me know if there are any questions.
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @c3024The text was updated successfully, but these errors were encountered: