Add ReportNameValuePairs key to Report Type#40661
Conversation
|
Hey @mountiny and @roryabraham, do you think you could take a look at this PR if you have a chance? I added the ReportNameKeys array to add some structure but I'm not sure iff there's a better way of doing it. |
|
From a quick look it seems good to me, but definitely add the rNVP names to CONST |
|
Ah okay, thanks @mountiny. What about what is there now? I did it that way to keep it consistent with the way the file is organized but it feels a little redundant. Because whenever we want to add a new key, we have to add it as keyName: "keyName". I think it would be easier to just add it as a list at the bottom. What do you think? List at the bottom meaning: |
|
I think that it might look redundant now, but in future we will want to use those keys elsewhere and its better to keep them in CONST |
|
Cool agreed. But we can also do it like this:
but keep it in CONST right? Which will avoid the redundancy? |
|
ah yeah we can do that too, although is there a case when new key would not be added to the keyName const you mentioned? |
I don't believe so right? If we need to pass the reportNameValuePair to App, then we'll add it and otherwise we won't? |
…nsify/App into srikar-reportNameValuePairs merge
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
So looking at this PR, what you've done is essentially:
- Created a runtime constant for all the names of the rNVPs
- then declared a compile-time constant that's essentially an object with all the rNVP names as keys and then
stringas the value
I think this is kind of backwards. What you can do is just declare a type for the reportNameValuePairs with all the names and the correct types. Reference this type whenever you need to - not sure when you'd need the constant with the rNVP names in a post-TS world. Simply this might be enough for what you're trying to do here.
diff --git a/src/types/onyx/Report.ts b/src/types/onyx/Report.ts
index 7b9e7de3ca..8c05e4007d 100644
--- a/src/types/onyx/Report.ts
+++ b/src/types/onyx/Report.ts
@@ -186,6 +186,10 @@ type Report = OnyxCommon.OnyxValueWithOfflineFeedback<
transactionThreadReportID?: string;
fieldList?: Record<string, PolicyReportField>;
+
+ reportNameValuePairs?: {
+ isArchived: boolean;
+ };
},
PolicyReportField['fieldID']
>;| // The minimum number of typed lines needed to enable the full screen composer | ||
| FULL_COMPOSER_MIN_LINES: 3, | ||
| }, | ||
| REPORT_NAME_KEYS: { |
There was a problem hiding this comment.
I'd recommend putting this under REPORT.NVPS rather than REPORT_NAME_KEYS
There was a problem hiding this comment.
I'm also wondering if the same thing we did for nameValuePairs we should do for reportNameValuePairs. cc @iwiznia
There was a problem hiding this comment.
Yes, but rNVPs should be _a bit simpler since we always want to send all of them, right?
Anyway, I assume it should be easy to do by queuing the update at the lowest level in auth, probably in Report::setNameValuePair
| fieldList?: Record<string, PolicyReportField>; | ||
|
|
||
| /** The reports name value pairs */ | ||
| reportNameValuePairs?: Record<ReportNameKeys, string>; |
There was a problem hiding this comment.
This type seems pretty sus - are all rNVP values strings?
| FULL_COMPOSER_MIN_LINES: 3, | ||
| }, | ||
| REPORT_NAME_KEYS: { | ||
| isArchived: 'isArchived', |
There was a problem hiding this comment.
Also, the keys in this file are typically always UPPER_SNAKE_CASE. You can extract all the camelCase values using ValueOf from type-fest.
|
If you did need a union of all rNVP names as a type (which, again, not sure you will), then you could do this: type ReportNameValuePairs = Report['reportNameValuePairs'];
type ReportNameValuePairsNames = keyof ReportNameValuePairs;If you did need a runtime constant for all the rNVP names (which, again, not sure you will), then you could do this: import type {ScreamingSnakeCase} from 'type-fest';
type KeysAsScreamingSnakeCase<T> = {
[K in keyof T as ScreamingSnakeCase<K>]: K;
};
// Note: this has to be declared again in full to make it available at runtime, but at least it's guaranteed to have all the correct keys and values.
const REPORT_NVP_NAMES: KeysAsScreamingSnakeCase<Report['reportNameValuePairs']> = {
IS_ARCHIVED: 'isArchived',
};it might be possible to do this in reverse such that:
but I wasn't able to quickly figure out a clean way to do that. The closest I got was something pretty ugly like this: const REPORT_NVPS = {
// Note: this should be boolean
IS_ARCHIVED: 'isArchived',
// Note: this should be a string
SOME_OTHER_NVP: 'myNVP',
} as const;
type ReportNameValuePairs = {
[Key in REPORT_NVPS]: Key extends 'isArchived'
? boolean
: Key extends 'myNVP'
? string
: never;
}; |
|
Doing this makes a lot of sense to me. It avoids redundancy, keeps everything in the same place, and allows for different types (you were right, not all of them are strings. I thought that when they got passed in from PHP they would be passed as strings but I'm wrong) And in that case, whenever we want to pass an Onyx Update to reportNameValuePairs, we would just send the whole object right? For example, in structureReportForOnyx in Auth, we would pass the whole reportNameValuePairs object and not only the fields we need. This is because we eventually will use most fields right? But, we would only add to reportNameValuePairs in App whenever we use a new report name value pair. So since we're only using reportNameValuePairs.isArchived in App right now, that's the only one that would be there. Does that sound good? |
|
@srikarparsi check out https://docs.google.com/document/d/1kubm8uFo2CTDfwB6_vhNL908cBN7qxDzNquIlfUW8S8/edit - we should do the same thing for reportNameValuePairs and move all logic that sends reportNameValuePairs to the front-end to a low level in Auth. |
|
Just read through @iwiznia's design doc. Just to make sure I'm understanding correctly, is this what you're suggesting: Currently, we send rNVP Onyx updates in two ways.
Is the suggestion to move only the 1st type of these rNVP Onyx Updates to a low level method like setNameValuePair? And then remove all the 1st type of rNVP Onyx Updates since these methods call setNameValuePair anyway to set the rNVP before sending the Onyx Update? |
|
If that’s the case, what would be the best way to handle this so that it doesn’t block on CLOSED/ARCHIVED? I’m thinking we keep this App PR so that we begin storing the isArchived rNVP correctly. This way, we won’t need to migrate it in the future. And then in the Auth, we send the isArchived Onyx update under reportNameValuePairs so that it’s stored correctly in App. And in the meanwhile, I can work on a design doc for rNVP Onyx Updates similar to the one Ioni did for NVPs? |
|
The idea is to always and only queue updates at the lowest level, so we don't have to queue updates everywhere and can forget about updates, since they are just handled automatically by the lower level methods. So all those you linked (and more) would be removed and instead would have just one call to queueOnyxUpdates in Report::setNameValuePair |
|
Cool, that makes sense. So what I'm thinking is:
Does that sound good? I could also do a design doc for steps 2 and 3 similar to @iwiznia's for NVPs. But am not sure if I should do it while working on Separate Archived/Closed functionality or after. |
|
@hoangzinh @ One of you needs to 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] |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
sounds like we've got consensus to store rNVPs under their own key, which is not the implementation we have in this PR
|
Yupp, working on a PR to introduce the new ReportNameValuePairs type in App and should have that ready tomorrow. |
|
I finished up most of it, just have to test a couple of things. I'm OOO until Wednesday for my friends' grad trip but I'll try to request a review on Monday or Tuesday. |
|
Hey! This is ready for re-review. I also incorporated the isArchived changes (the reason we're creating this new Onyx structure) so that it would be easier to test. But please let me know if it would be better to move this to a separate PR, I wasn't sure. But to test, I used this web branch which makes every OpenReport call push the reportNameValuePairs Onyx data with the isArchived key set. Then this App PR, replaces the composer with an archived banner if the isArchived value is true. I made sure that opening new reports, navigating away from them, and then returning to them replaces the composer box with the archived banner. Side note: We could probably use a useMemo to detect changes to reportNameValuePairs (similar to how we do it for report) so that navigating away and back isn't necessary but I wasn't sure how this would affect performance. I'll look into this more tomorrow but just wanted to re-request reviews since the core code for the ReportNameValuePairs Onyx structure is done. |
iwiznia
left a comment
iwiznia
left a comment
There was a problem hiding this comment.
What is the plan for all the rest of rNVPs?
| return reportNameValuePairs.isArchived; | ||
| } | ||
|
|
||
| return report?.statusNum === CONST.REPORT.STATUS_NUM.CLOSED && report?.stateNum === CONST.REPORT.STATE_NUM.APPROVED; |
There was a problem hiding this comment.
We need to delete this (and the report param) at some point, right?
There was a problem hiding this comment.
Yes we do, I put some information about that in the "Plan of action" section of his Design Doc
There was a problem hiding this comment.
ok please do not forget (I'd create an issue, but I have no memory)
I left a comment about it here. Does that sound good or am I missing anything? I can create issues for each rNVP and maybe we can add the Hot Pick label? Or I can work on it myself after finishing this Closed & Archived stuff. |
I would actually amend this a bit, as follows:
|
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
code for this PR looks good though
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.74-0 🚀
|
|
For migrating the other rNVPs, is there any preference between:
|
|
Both work for me, as long as we do it eventually 😄 |
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.74-6 🚀
|
5 participants
Details
Adding a key isArchived so that we can use it to denote archived reports instead of using the CLOSED state. Design Doc
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/387236
PROPOSAL:
Tests
Will be tested when we send the isArchived Onyx update in Auth.
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop