[No QA] [Workspace Feeds] Create new Card List page by VickyStash · Pull Request #44469 · Expensify/App

VickyStash · 2024-06-26T15:02:06Z

Details

[Workspace Feeds] Create new Card List page

Fixed Issues

$ #44310
PROPOSAL: N/A

Tests

Verify that no errors appear in the JS console

Pre-steps:

Add a new menu item to a workspace menu to simplify testing.
Insert this part of code into WorkspaceInitialPage screen.

protectedCollectPolicyMenuItems.push({
    translationKey: 'workspace.common.expensifyCard',
    icon: Expensicons.CreditCard,
    action: singleExecution(waitForNavigate(() => Navigation.navigate(ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID)))),
    routeName: SCREENS.WORKSPACE.EXPENSIFY_CARD,
});

You should see Expensify Card option in the workspace menu now.

Test steps:

Open Expensify Card page.
Make sure the page looks as expected.
Tap on info icon of Current Balance/Remaining Limit/Cash Back info blocks. Make sure popup with additional info appears. Remaining Limit popup should also have button to Require limit increase.

NOTE: the data is mocked for now, API endpoints are in progress.

Offline tests

N/A

QA Steps

Verify that no errors appear in the JS console

N/A

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

# Conflicts: # src/libs/API/parameters/index.ts # src/libs/Navigation/AppNavigator/Navigators/FullScreenNavigator.tsx # src/libs/actions/Policy/Policy.ts # src/pages/workspace/WorkspaceInitialPage.tsx

melvin-bot · 2024-06-27T13:52:41Z

@DylanDylann 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]

shawnborton · 2024-06-27T16:47:45Z

Looking good! For the limit column, can we use a currency symbol?

shawnborton · 2024-06-27T16:51:19Z

Can you share a quick screen recording when you get a chance as well? Thanks!

allgandalf · 2024-06-27T17:31:14Z

@VickyStash , I would be reviewing this PR instead of @DylanDylann , do let me know when you implement the latest suggestions from @shawnborton , I will start my review after the changes thanks

allgandalf

left some comments, will review the page UI once the suggestions from shawn are implemented

allgandalf · 2024-06-27T21:12:36Z

@@ -0,0 +1,6 @@
+type OpenPolicyExpensifyCardsPageParams = {
+    policyID: string;
+    authToken: string | null | undefined;


i don't think we should have a auth token ever as undefined, null case is okay but i highly doubt that we should keep it's type as undefined, what's your take on this @VickyStash ?

That's the return type of the getAuthToken function

App/src/libs/Network/NetworkStore.ts

Lines 96 to 98 in 1d607ac

function getAuthToken(): string | null | undefined {

return authToken;

}

But I agree that undefined can be removed, but I'm not sure that I should do it in this PR.

allgandalf · 2024-06-27T21:12:48Z

@@ -0,0 +1,6 @@
+type RequestExpensifyCardLimitIncreaseParams = {
+    authToken: string | null | undefined;


allgandalf · 2024-06-27T21:17:00Z

+    // const [cardsList] = useOnyx(`${ONYXKEYS.COLLECTION.EXPENSIFY_CARDS_LIST}${policyID}_Expensify Card`);
+    const cardsList = mockedCards;
+
+    const fetchExpensifyCards = useCallback(() => {


Isn't useMemo more appropriate here? , well there must be a reason for useCallback can you explain please

I wanted to keep it aligned with other places in the code:

App/src/pages/workspace/tags/WorkspaceTagsPage.tsx

Lines 59 to 61 in 1d607ac

const fetchTags = useCallback(() => {

Tag.openPolicyTagsPage(policyID);

}, [policyID]);

App/src/pages/workspace/WorkspaceInitialPage.tsx

Lines 136 to 138 in 1d607ac

const fetchPolicyData = useCallback(() => {

Policy.openPolicyInitialPage(route.params.policyID);

}, [route.params.policyID]);

allgandalf

Tested the latest changes, Look good to merge !

mountiny · 2024-07-02T09:09:52Z

+        /** Expensify cards list */
+        EXPENSIFY_CARDS_LIST: 'card_',


Suggested change

/** Expensify cards list */

EXPENSIFY_CARDS_LIST: 'card_',

/**

* Stores the card list for a given fundID and feed in the format: card_<fundID>_<bankName>

* So for example: card_12345_ExpensifyCard

*/

EXPENSIFY_CARDS_LIST: 'card_',

MariaHCD

Just have a question on removing the ExpensifyCard.ts in favor of Card.ts

https://github.com/Expensify/App/pull/44469/files#r1662157151

koko57 · 2024-07-02T09:17:34Z

@MariaHCD yes in a sec, totally haven't thought about it 😅

mountiny · 2024-07-02T09:20:15Z

+
+        /**
+         * Stores the card list for a given fundID and feed in the format: card_<fundID>_<bankName>
+         * So for example: card_12345_ExpensifyCard


Sorry, apparently its with a space

Suggested change

* So for example: card_12345_ExpensifyCard

* So for example: card_12345_Expensify Card

That is the card name right? <bankName> would be different here 🤔

@allgandalf it's not the name of the card, it's the name of the card feed which is referred to as bankName internally. For this project, the feed / bankName is Expensify Card - we will have more feeds introduced in the future.

ahh, now that makes more sense to me 👍

mountiny · 2024-07-02T09:25:15Z

+         * Stores the card list for a given fundID and feed in the format: card_<fundID>_<bankName>
+         * So for example: card_12345_ExpensifyCard
+         */
+        EXPENSIFY_CARDS_LIST: 'card_',


Not sure what is the best const name here so it doe snot intervene with the cardsList key

I dont think we should use Expensify_cards list as this will be reused for other feeds that are not necessarily Expensify cards cc @MariaHCD

I feel like Workspace might be ok, but also it might be be that clear with the domain feeds that are not strictly tied to a workspace

Suggested change

EXPENSIFY_CARDS_LIST: 'card_',

WORKSPACE_CARDS_LIST: 'card_',

Fair point. I'm thinking EXPENISFY_CARDS_LIST still makes the most sense.

Alternatively, we can go with WORKSPACE_CARDS_LIST and then for domain feeds it could a new const called DOMAIN_CARDS_LIST

But having just one makes the most sense, I think so I'm still fine with EXPENISFY_CARDS_LIST

Okay, looks like we're going with WORKSPACE_CARDS_LIST. I'm fine with that since it is more clear. We can change it down the line without much difficulty if we find it's not suitable for domain feeds.

mountiny

@koko57 couple comments

mountiny · 2024-07-02T09:46:43Z

+import type PersonalDetails from './PersonalDetails';
+
+/** Model of an Expensify card */
+type ExpensifyCard = OnyxCommon.OnyxValueWithOfflineFeedback<{


+1, I assume we could reuse the same model

koko57 · 2024-07-02T10:06:58Z

@mountiny addressed

allgandalf · 2024-07-02T10:56:26Z

are all the comments addressed @koko57 ? I will test once again on all platforms just to be sure

koko57 · 2024-07-02T11:00:26Z

@allgandalf yep, all addressed

allgandalf

Initial checklist is here, Tested again on all platforms after latest changes. Tests well on all platforms:

Screenshots/Videos

Android: Native

Screen.Recording.2024-07-02.at.6.15.28.PM.mov

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

mountiny

Thank you for addressing the feedback

mountiny · 2024-07-02T19:43:15Z

Seems like design also liked the changes so I am going to go ahead and merge this

OSBotify · 2024-07-02T19:51:53Z

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

OSBotify · 2024-07-03T20:02:03Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.4-0 🚀

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

OSBotify · 2024-07-04T22:14:30Z

🚀 Cherry-picked to staging by https://github.com/tgolen in version: 9.0.4-5 🚀

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

OSBotify · 2024-07-10T11:02:08Z

🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀

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

OSBotify · 2024-07-15T20:21:17Z

🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀

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

VickyStash added 10 commits June 21, 2024 16:06

Add a new WorkspaceCardPageFree screen and a route for it

30eb56d

Implemented WorkspaceCardListRow and WorkspaceCardListHeader components

93cf9a3

Implemented card list labels

071719d

Add onyx types and useOnyx hooks to get the data

18cae81

Add OpenPolicyExpensifyCardsPage API call

1468453

Merge branch 'main' into feature/expensify-card-list

c5917a4

# Conflicts: # src/libs/API/parameters/index.ts # src/libs/Navigation/AppNavigator/Navigators/FullScreenNavigator.tsx # src/libs/actions/Policy/Policy.ts # src/pages/workspace/WorkspaceInitialPage.tsx

Fix TS error

d179bbf

Merge branch 'main' into feature/expensify-card-list

23ac283

Add request limit increase button

e44f899

Lint fixes

8d197e6

VickyStash commented Jun 27, 2024

View reviewed changes

Comment thread src/languages/es.ts Outdated

VickyStash added 3 commits June 27, 2024 14:44

UI updates to fix ios display

c983f81

Clarify TODOs

9ff8407

Minor improvement

3d5e98c

VickyStash changed the title ~~[WIP] [Workspace Feeds] Create new Card List page~~ [No QA] [Workspace Feeds] Create new Card List page Jun 27, 2024

VickyStash marked this pull request as ready for review June 27, 2024 13:52

VickyStash requested review from a team as code owners June 27, 2024 13:52

melvin-bot Bot requested review from DylanDylann and removed request for a team June 27, 2024 13:52

VickyStash mentioned this pull request Jun 27, 2024

[Workspace Feeds] [External] Create new Card List page #44310

Closed

shawnborton reviewed Jun 27, 2024

View reviewed changes

Comment thread src/pages/workspace/expensifyCard/WorkspaceCardsListLabel.tsx Outdated

shawnborton reviewed Jun 27, 2024

View reviewed changes

Comment thread src/pages/workspace/expensifyCard/WorkspaceCardsListLabel.tsx Outdated

trjExpensify requested review from allgandalf and removed request for DylanDylann June 27, 2024 19:14

allgandalf reviewed Jun 27, 2024

View reviewed changes

koko57 added 2 commits July 1, 2024 19:06

fix: lint

94f13e6

fix: resolve conflicts

496fd20

allgandalf approved these changes Jul 1, 2024

View reviewed changes

fix: resolve conflicts

269314d

mountiny reviewed Jul 2, 2024

View reviewed changes

MariaHCD reviewed Jul 2, 2024

View reviewed changes

fix: update comment

9b4d832

mountiny reviewed Jul 2, 2024

View reviewed changes

fix: remove unused type

4fbe7ef

mountiny reviewed Jul 2, 2024

View reviewed changes

koko57 added 2 commits July 2, 2024 11:25

fix: minor fix

14f3606

fix: rename const

b000be0

mountiny reviewed Jul 2, 2024

View reviewed changes

fix: apply requested changes

e3e26c0

allgandalf approved these changes Jul 2, 2024

View reviewed changes

melvin-bot Bot requested a review from mountiny July 2, 2024 12:50

MariaHCD approved these changes Jul 2, 2024

View reviewed changes

mountiny approved these changes Jul 2, 2024

View reviewed changes

mountiny merged commit 5c71954 into Expensify:main Jul 2, 2024

github-actions Bot mentioned this pull request Jul 3, 2024

Deploy Checklist: New Expensify 2024-07-03 #44782

Closed

	function getAuthToken(): string \| null \| undefined {
	return authToken;
	}

		@@ -0,0 +1,6 @@
		type RequestExpensifyCardLimitIncreaseParams = {
		authToken: string \| null \| undefined;

	const fetchTags = useCallback(() => {
	Tag.openPolicyTagsPage(policyID);
	}, [policyID]);

	const fetchPolicyData = useCallback(() => {
	Policy.openPolicyInitialPage(route.params.policyID);
	}, [route.params.policyID]);

	* So for example: card_12345_ExpensifyCard
	* So for example: card_12345_Expensify Card

	EXPENSIFY_CARDS_LIST: 'card_',
	WORKSPACE_CARDS_LIST: 'card_',

Conversation

VickyStash commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Pre-steps:

Test steps:

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

Uh oh!

melvin-bot Bot commented Jun 27, 2024

Uh oh!

shawnborton commented Jun 27, 2024

Uh oh!

Uh oh!

Uh oh!

shawnborton commented Jun 27, 2024

Uh oh!

allgandalf commented Jun 27, 2024

Uh oh!

allgandalf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

allgandalf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaHCD left a comment

Choose a reason for hiding this comment

Uh oh!

koko57 commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaHCD Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koko57 commented Jul 2, 2024

Uh oh!

allgandalf commented Jul 2, 2024

VickyStash commented Jun 26, 2024 •

edited

Loading

koko57 commented Jul 2, 2024 •

edited

Loading

MariaHCD Jul 2, 2024 •

edited

Loading