Fix: Background report changes to Tags page after clicking Custom tag name in RHP #50416

twilight2294 · 2024-10-08T13:45:44Z

Details

Fixed Issues

$ #49883
PROPOSAL: #49883 (comment)

Tests

Precondition:

User is logged in to two devices
Workspace has tags

Open App
[Device A] Go to workspace chat
[Device A] Click + > Submit expense > Manual
[Device A] Proceed to confirmation page and click Tag
[Device B] Go to workspace settings > Tags
[Device B] Disable all tags
[Device A] Click Edit tags
[Device A] Click Settings > Custom tag name

Verify that The report in the background will remain after clicking Custom tag name

Verify that in general the whole flow tests well and we are navigated to the correct pages.

Verify that following the same steps above for categories, we are navigated correctly on category pages

Verify that no errors appear in the JS console

Offline tests

N/A can't test offline

QA Steps

Precondition:

User is logged in to two devices
Workspace has tags

Open App
[Device A] Go to workspace chat
[Device A] Click + > Submit expense > Manual
[Device A] Proceed to confirmation page and click Tag
[Device B] Go to workspace settings > Tags
[Device B] Disable all tags
[Device A] Click Edit tags
[Device A] Click Settings > Custom tag name

Verify that The report in the background will remain after clicking Custom tag name

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-10-07.at.8.45.54.PM.mov

Android: mWeb Chrome

Screen.Recording.2024-10-07.at.8.48.45.PM.mov

iOS: Native

Screen.Recording.2024-10-07.at.8.44.21.PM.mov

iOS: mWeb Safari

Screen.Recording.2024-10-07.at.8.43.29.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-10-07.at.7.58.45.PM.mov

MacOS: Desktop

Screen.Recording.2024-10-07.at.8.03.38.PM.mov

Signed-off-by: GitHub <noreply@github.com>

melvin-bot · 2024-10-08T15:15:55Z

@paultsimura Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

twilight2294 · 2024-10-08T15:18:41Z

@paultsimura there is an additional bug in this flow that when we refresh the page and then press back button the RHP will get dismissed, this is reproducible on production as well. I think we can fix it too in this PR, should we ask for a bounty increase for this issue ?

paultsimura · 2024-10-08T17:01:43Z

@paultsimura there is an additional bug in this flow that when we refresh the page and then press back button the RHP will get dismissed, this is reproducible on production as well. I think we can fix it too in this PR, should we ask for a bounty increase for this issue ?

Please add a recording – I cannot reproduce from this description.

paultsimura · 2024-10-08T17:23:31Z

@koko57 sorry for bothering you here – I just know you are well familiar with routing in the App. Could you please briefly look across this PR and see if it makes sense to you, or we could simplify it by somehow only changing a part of the Screens config 🙏

It honestly looks a bit bulky for me to duplicate every existing tag-related route just because we want to show it in RHP in one flow, and the main window in another flow.

twilight2294 · 2024-10-08T18:16:31Z

Please add a recording – I cannot reproduce from this description.

Screen.Recording.2024-10-08.at.11.45.43.PM.mov

Should be reproducible on staging (not in this PR as i already included the fix for that (not part of our PR though, just for demonstration)

koko57 · 2024-10-09T08:02:37Z

@paultsimura yeah, I agree it's a bit bulky. But if it's already implemented this way for categories, I cannot argue with this approach. Same for Expensify Card - I wonder if we should also implement it this way.

paultsimura · 2024-10-09T08:09:31Z

Thanks @koko57, maybe we'll come to some grand routing refactoring at some point that will allow supporting both full-page and RHP pages seamlessly.

paultsimura · 2024-10-09T08:11:27Z

@stitesExpensify we have noticed a couple of semi-related bugs here:

Bug 1:

Open App
Go to a workspace chat
Click + > Submit expense > Manual
Proceed to the confirmation page and click "Tag"
[Device B] Go to the workspace settings > Tags
[Device B] Disable all tags
Click Edit tags
Click Settings > Custom tag name
Refresh the page
Click the "Back" button

Expected: the user is navigated back to the previous page in the RHP with the possibility to continue the money request submission
Actual: the RHP is closed and the draft money request is dismissed

Bug 2:

Open App
Go to a workspace chat
Click + > Submit expense > Manual
Proceed to the confirmation page and click "Tag"
[Device B] Go to the workspace settings > Tags
[Device B] Disable all tags
Click Edit tags
Click one of the tags in the list
Click the tag name
Refresh the page
Modify the name and click "Save"

Expected: the changes are persisted and the user is navigated back to the previous page in the RHP with the possibility to continue the money request submission
Actual: the changes are persisted, but the RHP is closed and the draft money request is dismissed

Also, most scenarios we've fixed in this PR for Tags are still happening for Categories. Like refreshing the page on some steps, or clicking "Import spreadsheet" etc.

Would you be up to bumping the bounty to $500 here and let us take care of these flows more holistically?

stitesExpensify · 2024-10-10T17:37:28Z

Yep @paultsimura let's double it to $500 and get those fixed! CC @Christinadobrzyn

paultsimura · 2024-10-10T17:39:00Z

@twilight294 all yours again!

Signed-off-by: GitHub <noreply@github.com>

twilight2294 · 2024-10-10T20:38:05Z

@paultsimura can you review the updated code once? I will update the recordings after you have reviewed the code once

paultsimura · 2024-10-10T20:40:22Z

@twilight294 please test the Categories flow as well and fix where needed - it also misses some screens and I promised we'll fix them for the increased bounty.

twilight2294 · 2024-10-10T20:49:31Z

@twilight294 please test the Categories flow as well and fix where needed - it also misses some screens and I promised we'll fix them for the increased bounty.

It's a little late here, I will add categories tomorrow, can you review for tags in the meantime ? thanks 😄

paultsimura · 2024-10-10T21:04:22Z

It's midnight for me as well, will review tomorrow 👍

Signed-off-by: GitHub <noreply@github.com>

…o fixRHPIssues

Signed-off-by: GitHub <noreply@github.com>

twilight2294 · 2024-10-11T08:38:08Z

@twilight294 please test the Categories flow as well and fix where needed - it also misses some screens and I promised we'll fix them for the increased bounty.

Fixed @paultsimura

twilight2294 · 2024-10-14T09:35:40Z

@paultsimura missed to remove that extra param, thanks, I have updated the code, let me know if you have any more concerns

paultsimura · 2024-10-14T09:46:54Z

Actually @twilight294 this is gonna sound like a little more work, but it would be much better for the code clarity – it's quite confusing when we use the ternary operator like this:

Navigation.navigate(backTo ? route_1 : route_2)

Let's introduce a new variable on all these pages (isEmpty from 'lodash/isEmpty'):

const isQuickSettingsFlow = !isEmpty(backTo);

And use it instead of backTo, e.g.:

Navigation.goBack(isQuickSettingsFlow ? ROUTES.SETTINGS_TAGS_SETTINGS.getRoute(route?.params?.policyID, backTo) : ROUTES.WORKSPACE_TAGS_SETTINGS.getRoute(route?.params?.policyID));

Signed-off-by: GitHub <noreply@github.com>

…o fixRHPIssues

twilight2294 · 2024-10-14T11:09:43Z

thanks for the suggestions @paultsimura , implemented the changes

Signed-off-by: GitHub <noreply@github.com>

paultsimura · 2024-10-14T14:16:47Z

I'm sorry @twilight294 – I've double checked and it would be sufficient to use this instead of lodash:

const isQuickSettingsFlow = !!backTo;

This is the last change request, hopefully 🙏

paultsimura

+1 missed change.

paultsimura · 2024-10-14T14:34:59Z

src/pages/workspace/tags/ImportedTagsPage.tsx

@@ -126,7 +129,7 @@ function ImportedTagsPage({route}: ImportedTagsPageProps) {
        >
            <HeaderWithBackButton
                title={translate('workspace.tags.importTags')}
-                onBackButtonPress={() => Navigation.goBack(ROUTES.WORKSPACE_TAGS_IMPORT.getRoute(policyID))}
+                onBackButtonPress={() => Navigation.goBack(backTo ? ROUTES.SETTINGS_TAGS_IMPORT.getRoute(policyID, backTo) : ROUTES.WORKSPACE_TAGS_IMPORT.getRoute(policyID))}


Here as well, i guess:

Suggested change

onBackButtonPress={() => Navigation.goBack(backTo ? ROUTES.SETTINGS_TAGS_IMPORT.getRoute(policyID, backTo) : ROUTES.WORKSPACE_TAGS_IMPORT.getRoute(policyID))}

onBackButtonPress={() => Navigation.goBack(isQuickSettingsFlow ? ROUTES.SETTINGS_TAGS_IMPORT.getRoute(policyID, backTo) : ROUTES.WORKSPACE_TAGS_IMPORT.getRoute(policyID))}

missed it sorry

Signed-off-by: GitHub <noreply@github.com>

twilight2294 · 2024-10-14T14:47:38Z

I updated the code @paultsimura , got rid of the lodash

paultsimura

This is quite a big PR, I hope we didn't miss any test cases 🤞
LGTM ✔️

twilight2294 · 2024-10-16T18:39:01Z

bump for review @stitesExpensify

stitesExpensify · 2024-10-17T18:03:29Z

I have been OOO but am working my way through this. Lots of changes!

paultsimura · 2024-10-21T10:52:32Z

@stitesExpensify any progress with this one? It's likely to accumulate conflicts over time because there are a lot of changes.

OSBotify · 2024-10-23T00:08:05Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-10-23T16:43:30Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.0.53-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

github-actions · 2024-10-23T17:44:58Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.0.53-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Julesssss · 2024-10-24T15:26:01Z

Hey all, it seems like this PR might have introduced a small issue with Tag pages navigation

twilight2294 · 2024-10-24T15:27:15Z

Looking.....

github-actions · 2024-10-25T02:16:53Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.53-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

github-actions · 2024-10-25T07:47:57Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.53-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

paultsimura · 2024-10-27T19:17:03Z

@twilight2294 what are your findings on this?

twilight2294 · 2024-10-27T19:18:47Z

Sorry, it's weekend so didn't reply early, found a fix, i will raise PR tomorrow

twilight2294 added 4 commits October 8, 2024 13:29

Add new routes for tags RHP

c45fe04

Signed-off-by: GitHub <noreply@github.com>

Background report changes to Tags page after clicking Tag pages

a379e49

Signed-off-by: GitHub <noreply@github.com>

Merge branch 'main' into fixRHPIssues

86cd87b

Fix rightModal navigator

a51d9b4

Signed-off-by: GitHub <noreply@github.com>

twilight2294 mentioned this pull request Oct 8, 2024

Fix: Background report changes to Tags page after clicking Custom tag name in RHP #50332

Closed

50 tasks

twilight2294 marked this pull request as ready for review October 8, 2024 15:15

twilight2294 requested a review from a team as a code owner October 8, 2024 15:15

melvin-bot bot requested review from paultsimura and removed request for a team October 8, 2024 15:15

twilight2294 mentioned this pull request Oct 9, 2024

[HOLD for payment 2024-11-01] [$500] Tags - Background report changes to Tags page after clicking Custom tag name in RHP #49883

Open

6 tasks

Fix navigation issues on all tag screens

4f38b4e

Signed-off-by: GitHub <noreply@github.com>

add backTo as dependency

671179a

twilight2294 added 3 commits October 11, 2024 08:29

Fix routes for RHP on categories

8694a58

Signed-off-by: GitHub <noreply@github.com>

Merge branch 'fixRHPIssues' of https://github.com/twilight294/App int…

19d8f83

…o fixRHPIssues

add dependency and use

92f5990

Signed-off-by: GitHub <noreply@github.com>

twilight2294 added 2 commits October 14, 2024 11:04

add isQuickSettingsFlow const for clarity in ternary operator

887c85c

Signed-off-by: GitHub <noreply@github.com>

Merge branch 'fixRHPIssues' of https://github.com/twilight294/App int…

7e497a6

…o fixRHPIssues

twilight2294 added 2 commits October 14, 2024 11:13

add backTo as dependency

39130e3

Signed-off-by: GitHub <noreply@github.com>

fix wrong dependency import

82f5ba0

Signed-off-by: GitHub <noreply@github.com>

paultsimura suggested changes Oct 14, 2024

View reviewed changes

get ride of lodash

1dd6f99

Signed-off-by: GitHub <noreply@github.com>

paultsimura approved these changes Oct 14, 2024

View reviewed changes

melvin-bot bot requested a review from stitesExpensify October 14, 2024 15:23

stitesExpensify approved these changes Oct 22, 2024

View reviewed changes

stitesExpensify merged commit faf48fc into Expensify:main Oct 22, 2024
17 of 18 checks passed

github-actions bot mentioned this pull request Oct 23, 2024

Deploy Checklist: New Expensify 2024-10-23 #51344

Closed

42 tasks

ntdiary mentioned this pull request Oct 28, 2024

[$250] Tags - App returns to tag page after tapping back button on workspace editor #51414

Open

8 tasks

paultsimura mentioned this pull request Nov 1, 2024

Fix - RHN throws error if tags are deleted by admin while member is selecting #49506

Merged

50 tasks

	onBackButtonPress={() => Navigation.goBack(backTo ? ROUTES.SETTINGS_TAGS_IMPORT.getRoute(policyID, backTo) : ROUTES.WORKSPACE_TAGS_IMPORT.getRoute(policyID))}
	onBackButtonPress={() => Navigation.goBack(isQuickSettingsFlow ? ROUTES.SETTINGS_TAGS_IMPORT.getRoute(policyID, backTo) : ROUTES.WORKSPACE_TAGS_IMPORT.getRoute(policyID))}

Fix: Background report changes to Tags page after clicking Custom tag name in RHP #50416

Fix: Background report changes to Tags page after clicking Custom tag name in RHP #50416

Conversation

twilight2294 commented Oct 8, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Oct 8, 2024

twilight2294 commented Oct 8, 2024

paultsimura commented Oct 8, 2024

paultsimura commented Oct 8, 2024

twilight2294 commented Oct 8, 2024 • edited Loading

koko57 commented Oct 9, 2024

paultsimura commented Oct 9, 2024

paultsimura commented Oct 9, 2024

stitesExpensify commented Oct 10, 2024

paultsimura commented Oct 10, 2024

twilight2294 commented Oct 10, 2024

paultsimura commented Oct 10, 2024

twilight2294 commented Oct 10, 2024

paultsimura commented Oct 10, 2024

twilight2294 commented Oct 11, 2024

twilight2294 commented Oct 14, 2024

paultsimura commented Oct 14, 2024

twilight2294 commented Oct 14, 2024

paultsimura commented Oct 14, 2024

paultsimura left a comment

Choose a reason for hiding this comment

paultsimura Oct 14, 2024

Choose a reason for hiding this comment

twilight2294 Oct 14, 2024

Choose a reason for hiding this comment

twilight2294 commented Oct 14, 2024

paultsimura left a comment

Choose a reason for hiding this comment

twilight2294 commented Oct 16, 2024

stitesExpensify commented Oct 17, 2024

paultsimura commented Oct 21, 2024 • edited Loading

OSBotify commented Oct 23, 2024

github-actions bot commented Oct 23, 2024

github-actions bot commented Oct 23, 2024

Julesssss commented Oct 24, 2024

twilight2294 commented Oct 24, 2024

github-actions bot commented Oct 25, 2024

github-actions bot commented Oct 25, 2024

paultsimura commented Oct 27, 2024

twilight2294 commented Oct 27, 2024

twilight2294 commented Oct 8, 2024 •

edited

Loading

twilight2294 commented Oct 8, 2024 •

edited

Loading

paultsimura commented Oct 21, 2024 •

edited

Loading