Fix for Not found page appearing for private notes list, view and edit pages in deep link #29636

salonikumawat28 · 2023-10-15T02:54:36Z

Details

As discussed in the issue, we are doing following:

Created a HOC called withReportAndPrivateNotesOrNotFound which loads report and private notes for it. It shows not found view if report and private notes are not found. It shows loading indicator if the report or private notes are still loading.
We are using withReportAndPrivateNotesOrNotFound for list, edit and view page of private notes so that we make sure that report and private notes are available. This makes sure that not found view is not shown when private notes pages are accessed via deep link.
We have removed not found view and loading indicator views from the list, edit and view pages as both of these will now be handled by HOC withReportAndPrivateNotesOrNotFound.
Offline mode behaviour is now made consistent for list, view and edit pages for private notes.

Fixed Issues

$ #27704
PROPOSAL: #27704 (comment)

Tests

On web and mweb platforms:

Create a report and go to Private notes and add a private note.
Open list, view and edit pages of private note in normal way to ensure loading indicator, not found view and private notes are shown as expected. Here list URL is: localhost:8082/r/:reportId/notes, view URL is: localhost:8082/r/:reportId/notes/:accountId and edit URL is: localhost:8082/r/:reportId/notes/:accountId/edit.
Now open each one of these URLs as deep link. Every time you try, first logout so that cache gets cleared. Verify that Not found view and loading indicator doesn't show unexpected behaviour i.e. Not found view should not appear for a valid private note, loading indicator should not be shown indefinitely etc.

On native platforms: Repeat step 1 and 2 from above.

Verify that no errors appear in the JS console

Offline tests

Try following scenarios:

Report and private notes both are not cached locally + mode is offline:
Steps:
a. Login in online mode without going to the report
b. Go offline
c. Go to notes normally or via deep link
d. loading indicator should appear for all list, view and edit pages of private notes.
Report is cached but private notes are not cached locally + mode is offline:
Steps:
a. Login in online mode and go to the report.
b. Go offline
c. Go to notes normally or via deep link
d. loading indicator should appear for all list, view and edit pages of private notes.
Both report and private notes are cached locally + mode is offline:
Steps:
a. Login in online mode, go to the report and then go to the notes.
b. Go offline
c. Go to notes normally or via deep link
d. Private note should appear

QA Steps

Try following scenarios:

Normal flow + valid report and private notes:
Steps:
a. Login, go to report, and then go to private notes in normal way.
b. Verify list, edit and view pages of private notes should show private notes after few seconds of loading indicator.
Deep link flow + valid report and private notes:
Steps:
a. Logout from Expensify
b. Login using deep link of list page of private notes.
c. Verify private notes are shown after few seconds of loading indicator.
d. Repeat a, b & c for view and edit pages.
Deep link flow + invalid report or private note:
a. Logout from Expensify
b. Login using deep link of list, view or edit page of private notes. Ensure that either report id is incorrect or note's accountID is incorrect.
c. Verify Not found view is shown after few seconds of loading indicator view.
Open same account from localhost and staging, and change private note from one and verify that updates are reflected in another.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android-native.mp4

Android: mWeb Chrome

android-chrome.mp4

iOS: Native

ios-native.mp4

iOS: mWeb Safari

ios-safari.mp4

MacOS: Chrome / Safari

mac-chrome.mp4

MacOS: Desktop

mac-desktop.mp4

melvin-bot · 2023-10-15T02:54:41Z

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

github-actions · 2023-10-15T02:54:52Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

salonikumawat28 · 2023-10-15T02:57:02Z

I have read the CLA Document and I hereby sign the CLA

salonikumawat28 · 2023-10-15T02:57:16Z

recheck

salonikumawat28 · 2023-10-16T13:56:39Z

@ArekChr What do you think about this pull request?

ArekChr

overall looks good, left some comments

ArekChr · 2023-10-17T10:25:10Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+    function WithReportAndPrivateNotesOrNotFound(props) {
+        const {route, report, network, session} = props;


Suggested change

function WithReportAndPrivateNotesOrNotFound(props) {

const {route, report, network, session} = props;

function WithReportAndPrivateNotesOrNotFound({route, report, network, session}) {

@ArekChr As this is HOC, I am passing the all the props to WrapperComponent so I need all of them:

const rest = _.omit(props, ['forwardedRef']); return ( <WrappedComponent // eslint-disable-next-line react/jsx-props-no-spreading {...rest} ref={props.forwardedRef} /> );

So I can't move const {route, report, network, session} = props; in component arguments.
What do you think?

Now I understand your point of view. It makes sense. But it could be slightly improved, what do you think about spreading rest props here instead?

function WithReportAndPrivateNotesOrNotFound({ forwardedRef , ...props}) {

Then we will no need to use underscore omit and pass props with spread inside WrapperComponent

@ArekChr This is a good idea in general but if I do it, what should i replace {...rest} with? I want to pass all the properties to WrappedComponent as props (except forwardedRef). As we already spreaded props, I can't pass it to WrappedComponent anymore. Is there a solution for this?

<WrappedComponent // eslint-disable-next-line react/jsx-props-no-spreading {...rest} ref={forwardedRef} />

You will replace it with {...props}, or you can name it {...rest} instead {...props} for better readability, the idea is to omit and forwardedRef from rest props by not using underscore.

@ArekChr I see what you are saying.
To omit underscore usage, here is what I can do based on your suggestion:

Option 1
Changing to:

function WithReportAndPrivateNotesOrNotFound({ forwardedRef , ...rest}) { . . <WrappedComponent // eslint-disable-next-line react/jsx-props-no-spreading {...rest} ref={forwardedRef} />

In this case, I need to replace all the route usage with rest.route. I need to do the same with report, network and session objects. This is because route is not defined now. If I do this, its not that readable.

Option 2:
Changing to:

function WithReportAndPrivateNotesOrNotFound({ forwardedRef , ...props}) { . . <WrappedComponent // eslint-disable-next-line react/jsx-props-no-spreading {...props} ref={forwardedRef} />

This is same as above. Better than option 1 in readability but I am repeating the props. everywhere now to access route, report, network and session. To avoid this, I originally used const {route, report, network, session} = props;. But I can go with this option if this is what you suggest is better.

Option 3:
Changing to:

function WithReportAndPrivateNotesOrNotFound({ forwardedRef, route, report, network, session, ...props}) { . . <WrappedComponent // eslint-disable-next-line react/jsx-props-no-spreading {route, report, network, session, ...props} ref={forwardedRef} />

In this case, I am getting route, report, network, session in input arg and then getting it constructing it back in WrappedComponent.

I see these 3 options to not use underscore and use route, report, network, session for my usecase and also pass it them to WrappedComponent along with other props. Should I go with Option 2?

By the way, to keep in mind for future usecases, can you tell me why we are avoiding _.omit? Should we skip it in general? Is it because it creates a shallow copy (still a copy) of the object?

@salonikumawat28 Option 2 looks good to me. We can it combine with your current approach so you can destruct in props and below the props as it is now.

So it will look like this:

function WithReportAndPrivateNotesOrNotFound({ forwardedRef , ...props}) { const {route, report, network, session} = props; ... <WrappedComponent // eslint-disable-next-line react/jsx-props-no-spreading { ...props } ref={forwardedRef} />

Regarding underscore, this lib is removed while TypeScript migration so we will not use this anymore in future.

Made the changes. Thanks.

ArekChr · 2023-10-17T10:26:23Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+        const shouldShowFullScreenLoadingIndicator = isLoadingPrivateNotes !== false && isPrivateNotesEmpty;
+
+        // eslint-disable-next-line rulesdir/no-negated-variables
+        const shouldShowNotFoundPage = useMemo(() => {


Why this value is memoized?

@ArekChr I am calling ReportUtils.isArchivedRoom(report). In addition, I am doing conditional checks in the memoized function. I dont want this to be re-computed every time until deps change.

ArekChr · 2023-10-17T10:27:38Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+import ONYXKEYS from '../../../ONYXKEYS';
+
+export default function (WrappedComponent) {
+    const propTypes = {


Move propTypes and default props outside of this function

@ArekChr There are 2 functions in HOC. One is outside function representing HOC and another function within representing enhanced wrapper component. propTypes and defaultProps are already outside the enhanced component but they are within the HOC function.
i.e.

https://github.com/Expensify/App/blob/98b6f7594b9bf88f135609155b19eb348acbf572/src/pages/home/report/withReportAndPrivateNotesOrNotFound.js#L18-L56

i.e. its outside function defined at line 55-56 but inside function defined at line 18-19.

This is similar to other HOC like withReportOrNotFound and withReportAndReportActionOrNotFound. Example snippet of withReportOrNotFound:

App/src/pages/home/report/withReportAndReportActionOrNotFound.js

Lines 18 to 80 in 651be71

export default function (WrappedComponent) {

const propTypes = {

/** The HOC takes an optional ref as a prop and passes it as a ref to the wrapped component.

* That way, if a ref is passed to a component wrapped in the HOC, the ref is a reference to the wrapped component, not the HOC. */

forwardedRef: PropTypes.func,

/** The report currently being looked at */

report: reportPropTypes,

/** The report metadata */

reportMetadata: reportMetadataPropTypes,

/** Array of report actions for this report */

reportActions: PropTypes.shape(reportActionPropTypes),

/** The policies which the user has access to */

policies: PropTypes.objectOf(

PropTypes.shape({

/** The policy name */

name: PropTypes.string,

/** The type of the policy */

type: PropTypes.string,

}),

),

/** Route params */

route: PropTypes.shape({

params: PropTypes.shape({

/** Report ID passed via route */

reportID: PropTypes.string,

/** ReportActionID passed via route */

reportActionID: PropTypes.string,

}),

}).isRequired,

/** Beta features list */

betas: PropTypes.arrayOf(PropTypes.string),

/** Indicated whether the report data is loading */

isLoadingReportData: PropTypes.bool,

/** Is the window width narrow, like on a mobile device? */

isSmallScreenWidth: PropTypes.bool.isRequired,

};

const defaultProps = {

forwardedRef: () => {},

reportActions: {},

report: {},

reportMetadata: {

isLoadingReportActions: false,

isLoadingMoreReportActions: false,

},

policies: {},

betas: [],

isLoadingReportData: true,

};

// eslint-disable-next-line rulesdir/no-negated-variables

function WithReportAndReportActionOrNotFound(props) {

const getReportAction = useCallback(() => {

I see we have 2 different approaches in the codebase. However, If you define propTypes and defaultProps inside the component, they'll be redefined every time the component is invoked, which is not efficient.

@ArekChr Sorry for the confusion. I want to ask further clarification. Is your proposal is to move the propTypes and defaultProps inside the component or outside the component?
In first comment, you mentioned moving it outside of the function:

Move propTypes and default props outside of this function

In new comment, you mentioned that defining the propTypes and defaultProps outside the component makes it inefficient as props are redefined every time the component is invoked.

I see we have 2 different approaches in the codebase. However, If you define propTypes and defaultProps outside the component, they'll be redefined every time the component is invoked, which is not efficient.

My understanding is that we should not define the propTypes and defaultProps inside component because they will be redefined every time the component is invoked. Is this understanding not correct?

Assuming my understanding is correct, I am defining the propTypes and defaultProps outside the React component. In below example, the component is defined at line 55. The function defined in line 18 is not React component, it's the HOC function. The propTypes and defaultProps are outside React component where the component is defined in line 55.

https://github.com/Expensify/App/blob/98b6f7594b9bf88f135609155b19eb348acbf572/src/pages/home/report/withReportAndPrivateNotesOrNotFound.js#L18-L56

Let me know if this explanation and understanding correct?

Sorry for the confusion, I had a typo in the previous message, I mean inside, if we define propTypes inside HOC these props will be recreated each time per instance, but this is just a little thing, props are not heavy, so we can leave as it is.

@ArekChr Thanks for the clarifications. I understand your point now. I have moved it outside.

ArekChr · 2023-10-19T07:09:02Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mweb.chrome.mov

Mobile Web - Safari

mweb.safari.mov

Desktop

desktop.mov

iOS

ios.mov

ios2.mov

Android

android.mov

ArekChr · 2023-10-19T07:10:21Z

@salonikumawat28 I found a bug. When navigating to private notes, a "not found" page is displayed in case I don't have any private notes.

Nagranie.z.ekranu.2023-10-19.o.09.08.12.mov

salonikumawat28 · 2023-10-19T08:39:13Z

@salonikumawat28 I found a bug. When navigating to private notes, a "not found" page is displayed in case I don't have any private notes.

Nagranie.z.ekranu.2023-10-19.o.09.08.12.mov

@ArekChr Thanks for catching the issue. I have fixed this and tested following scenarios again:

Reports with and without notes - Tried with and without deep links - Tried list, view and edit pages
Invalid account Id in URL to verify not found view - Tried with and without deep links - Tried all private notes pages
Invalid report id in URL to verify not found view - Tried with and without deep links - Tried all private notes pages.
Tried offline mode for all the private notes pages.

ArekChr · 2023-10-20T11:22:13Z

@salonikumawat28 Thanks for update 🙌🏼 Testing

ArekChr · 2023-10-20T12:30:42Z

@salonikumawat28, I found another bug but it appears to be unrelated to the current issue. When I navigate to http://localhost:8082/r/:workspaceid/notes/:accountid/edit to edit private notes and then go back, the workspaceId changes to 'undefined'. I've observed this behaviour on both mweb (Safari/Chrome) and the web. I also verified that it happens on staging. If this isn't a quick fix, perhaps we should open a separate issue.

Overall I tested all platforms and the current issue is fixed and works as expected 🙌🏼

mweb.safari.mov

ArekChr · 2023-10-20T12:35:33Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+            <WrappedComponent
+                // eslint-disable-next-line react/jsx-props-no-spreading
+                {...props}
+                ref={props.forwardedRef}


forwardedRef is destructed in line 55, it no longer exists in props.

Suggested change

ref={props.forwardedRef}

ref={forwardedRef}

@ArekChr my bad. Fixed. thanks.

salonikumawat28 · 2023-10-20T13:39:16Z

@salonikumawat28, I found another bug but it appears to be unrelated to the current issue. When I navigate to http://localhost:8082/r/:workspaceid/notes/:accountid/edit to edit private notes and then go back, the workspaceId changes to 'undefined'. I've observed this behaviour on both mweb (Safari/Chrome) and the web. I also verified that it happens on staging. If this isn't a quick fix, perhaps we should open a separate issue.

Overall I tested all platforms and the current issue is fixed and works as expected 🙌🏼

mweb.safari.mov

@ArekChr I also checked that this issue is also present in staging. I suggest to take this issue separately as it's not similar to existing issue. What do you think? Once the PR merges, I can create this issue if that's fine with you.

@ArekChr If PR looks good to you, this can be approved now?

salonikumawat28 · 2023-10-23T06:24:48Z

@ArekChr If everything looks good to you, can this PR be merged to avoid penalty? 6 business days penalty is approaching in few hours on this PR.

ArekChr

All good, Thanks for PR!

salonikumawat28 · 2023-10-25T08:28:13Z

@MonilBhavsar Can you have a look at the PR if it looks good?
PR is currently under penalty of 50% and the contract will terminate in a day as per this link :(

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

MonilBhavsar · 2023-10-25T09:06:53Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+            }
+
+            Report.getReportPrivateNote(report.reportID);
+            // eslint-disable-next-line react-hooks/exhaustive-deps -- do not add isLoadingPrivateNotes to dependencies


do not add isLoadingPrivateNotes to dependencies

Why though?

Fixed. thanks

@MonilBhavsar Reverted this change as having deps on isLoadingPrivateNotes causes infinite loop as every time isLoadingPrivateNotes changes, we fetch the API again which then again changes the isLoadingPrivateNotes which creates infinite loop.

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

MonilBhavsar · 2023-10-25T09:09:06Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+    function WithReportAndPrivateNotesOrNotFound({forwardedRef, ...props}) {
+        const {route, report, network, session} = props;
+        const accountID = route.params.accountID;
+        const isLoadingPrivateNotes = report.isLoadingPrivateNotes;


Is isLoadingPrivateNotes always set or we should default to false if it doesn't exist?

@MonilBhavsar No I dont want to set it to true or false. I want to keep it undefined until useEffect fetches the private notes and post fetching it sets it true or false.

Having three values of isLoadingPrivateNotes: undefined, true and false helps in knowing following:

If isLoadingPrivateNotes is undefined or true, and the private notes are empty, this means that we are yet to load private notes OR the private notes are still loading. So in these cases, show loading indicator.

If isLoadingPrivateNotes is defined and false, then it means we fetched private notes, so dont shon loading indicator even if private notes are empty as that's a valid case for user to not have private notes.

That's the reason why I am using isLoadingPrivateNotes !== false instead of isLoadingPrivateNotes

I feel like we can skip undefined case and set isLoadingPrivateNotes to true if we're about to, or fetching notes and then render loading indicator

done. thanks.

MonilBhavsar · 2023-10-25T09:09:37Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+        }, [report.reportID, network.isOffline]);
+
+        const isPrivateNotesEmpty = accountID ? _.isEmpty(lodashGet(report, ['privateNotes', accountID, 'note'], '')) : _.isEmpty(report.privateNotes);
+        const shouldShowFullScreenLoadingIndicator = isLoadingPrivateNotes !== false && isPrivateNotesEmpty;


Can be simplified I think

Suggested change

const shouldShowFullScreenLoadingIndicator = isLoadingPrivateNotes !== false && isPrivateNotesEmpty;

const shouldShowFullScreenLoadingIndicator = isLoadingPrivateNotes && isPrivateNotesEmpty;

Check above comment for details.

isLoadingPrivateNotes !== false give me true when isLoadingPrivateNotes is undefined or when isLoadingPrivateNotes is true which is what i need.

done. thanks.

MonilBhavsar · 2023-10-25T09:10:29Z

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

+            }
+
+            // Don't show not found view if the notes are still loading, or if the notes are non-empty.
+            if (isLoadingPrivateNotes !== false || !isPrivateNotesEmpty) {


same here

Suggested change

if (isLoadingPrivateNotes !== false || !isPrivateNotesEmpty) {

if (isLoadingPrivateNotes || !isPrivateNotesEmpty) {

Same comment as above.

done. thanks.

src/pages/home/report/withReportAndPrivateNotesOrNotFound.js

salonikumawat28 · 2023-10-25T10:05:03Z

@MonilBhavsar Answered all comments. Can you have a look?

salonikumawat28 · 2023-10-25T10:09:15Z

@MonilBhavsar 2 of my starting commits on 13 oct are not signed causing failures. Is there a documentation I can refer to sign those?

MonilBhavsar · 2023-10-25T10:13:25Z

Check out a new branch to test on in case something bad happens
Find the commit before the one you want to change
git rebase -i --preserve-merges <sha>
Change pick to edit for the commit you want to sign
git commit -S --amend --no-edit when it prompts you
git rebase --continue
Push your new branch to github, confirm that it is fixed
Optional: Repeat steps 3-6 on your actual branch and force push. Otherwise just close the old PR and create a new one with the new test branch

src/pages/PrivateNotes/PrivateNotesViewPage.js

MonilBhavsar

Looks good to me except we're now making multiple redundant API calls - GetReportPrivateNote. This behavior doesn't exist on main branch

Screen.Recording.2023-10-26.at.8.23.51.PM.mov

src/pages/PrivateNotes/PrivateNotesViewPage.js

salonikumawat28 · 2023-10-26T16:56:43Z

Looks good to me except we're now making multiple redundant API calls - GetReportPrivateNote. This behavior doesn't exist on main branch

Screen.Recording.2023-10-26.at.8.23.51.PM.mov

@MonilBhavsar Corrected this and verified in offline and non-offline mode for all private pages. I am using _.isUndefined calls though. Is it fine to use it? I see the method is used in repo multiple times but recently ArekChr mentioned to avoid underscore library.

MonilBhavsar · 2023-10-26T17:13:35Z

Using underscore lib is fine. @ArekChr any specific reason why we should not use it?
Also could you please take a look at he changes after you approved, thanks!

MonilBhavsar · 2023-10-26T17:16:06Z

Looks like this change to only use boolean for isLoadingPrivateNotes was reverted and that's why we're checking checking undefined for isLoadingPrivateNotes - #29636 (comment). Is that right?

salonikumawat28 · 2023-10-26T18:27:24Z

isLoadingPrivateNotes

@MonilBhavsar I want different behaviours when isLoadingPrivateNotes is undefined, true or false. So i dont want to set any default value to it.
For example: If isLoadingPrivateNotes is undefined, it means that we haven't fetched private notes yet. So in this case, I want to start fetching private notes. But if isLoadingPrivateNotes is defined be it true or false, I don't want to fetch private notes again.

MonilBhavsar · 2023-10-26T18:33:11Z

IMO having a different variable to hold that value would be better rather than deriving that from different related variable. What do you think?

salonikumawat28 · 2023-10-26T20:32:25Z

IMO having a different variable to hold that value would be better rather than deriving that from different related variable. What do you think?

@MonilBhavsar I have set !_.isUndefined(report.isLoadingPrivateNotes) to a different variable and using it. Is this what you meant? Let me know if you meant something else.

MonilBhavsar · 2023-10-30T15:04:29Z

Yes, that's better, thanks!
@ArekChr if you could please take a look at new changes and also test it, thank you!

Also, we have conflicts to resolve.

ArekChr · 2023-10-31T12:25:11Z

Reviewed and retested. Overall, everything works for me as expected.

We need to update the latest main branch and retest because are 1300 commits behind it.

ArekChr · 2023-10-31T12:26:23Z

I will be OOO for the remainder of this week and return on Monday.

salonikumawat28 · 2023-11-06T11:29:03Z

@MonilBhavsar Can you approve the PR if it looks good to you? All merge conflicts are resolved and new changes were reviewed by @ArekChr

MonilBhavsar · 2023-11-06T12:17:14Z

Thanks and looking

new changes were reviewed by @ArekChr

Sorry, where was this confirmed. I missed it

salonikumawat28 · 2023-11-06T12:19:32Z

Reviewed and retested. Overall, everything works for me as expected.

We need to update the latest main branch and retest because are 1300 commits behind it.

@MonilBhavsar I was referring to this comment from @ArekChr. I haven't made any new changes post this comment. I merged with main branch and retested on my end though post this comment.

On another note, is it possible to run the workflows as author to verify all checks are passing?

MonilBhavsar

Looks good overall and works fine

MonilBhavsar · 2023-11-06T17:39:46Z

src/pages/home/report/withReportOrNotFound.tsx

@@ -36,7 +36,7 @@ export default function (
            const isReportIdInRoute = props.route.params.reportID?.length;

            if (shouldRequireReportID || isReportIdInRoute) {
-                const shouldShowFullScreenLoadingIndicator = props.isLoadingReportData && (!Object.entries(props.report ?? {}).length || !props.report?.reportID);
+                const shouldShowFullScreenLoadingIndicator = props.isLoadingReportData !== false && (!Object.entries(props.report ?? {}).length || !props.report?.reportID);


Sorry, why this change was made?

@MonilBhavsar When I took merge a week back, I found that someone made a regression when renaming the file from js to tsx.
In old withReportOrNotFound.js, we had default props setting isLoadingReportData to true but the default props were removed in new tsx file.

This change is required when default props is not provided. This changes fixes the regression caused by removing default props at all places which uses withReportOrNotFound.tsx.

What I propose is to unblock this PR with this change and then revisiting on how to bring back the default props in tsx. I think adding back default props should be out of scope of this PR. What are your thoughts on this?

Thanks for clarifying. Sounds good to make a follow up PR

MonilBhavsar

Looks good and works fine
Thank you!

OSBotify · 2023-11-06T20:00:17Z

✋ 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 · 2023-11-06T22:29:52Z

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.96-0 🚀

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

OSBotify · 2023-11-09T22:47:25Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.96-15 🚀

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

OSBotify · 2023-11-10T15:43:57Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.97-7 🚀

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

salonikumawat28 requested a review from a team as a code owner October 15, 2023 02:54

melvin-bot bot requested review from ArekChr and removed request for a team October 15, 2023 02:54

salonikumawat28 mentioned this pull request Oct 15, 2023

[$500] Web - Not found page display briefly when we access the PrivateNotesListPage via deeplink #27704

Closed

6 tasks

ArekChr reviewed Oct 17, 2023

View reviewed changes

ArekChr reviewed Oct 20, 2023

View reviewed changes

salonikumawat28 requested a review from ArekChr October 23, 2023 06:26

ArekChr approved these changes Oct 23, 2023

View reviewed changes

melvin-bot bot requested a review from MonilBhavsar October 23, 2023 11:47

MonilBhavsar reviewed Oct 25, 2023

View reviewed changes

salonikumawat28 added 4 commits October 25, 2023 15:28

merge main

68c00b9

Merge branch 'main' of https://github.com/Expensify/App

f0e9684

Fix for private notes deep link showing not found view

0046065

Merge branch 'main' of https://github.com/Expensify/App into issue_27704

f5921f1

MonilBhavsar reviewed Oct 26, 2023

View reviewed changes

src/pages/PrivateNotes/PrivateNotesViewPage.js Show resolved Hide resolved

salonikumawat28 added 2 commits October 26, 2023 09:52

Merge branch 'main' of https://github.com/Expensify/App into issue_27704

189e4b3

Resolving merge issue

6fd24b1

MonilBhavsar reviewed Oct 26, 2023

View reviewed changes

src/pages/PrivateNotes/PrivateNotesViewPage.js Show resolved Hide resolved

salonikumawat28 added 2 commits October 26, 2023 18:13

Merge branch 'main' of https://github.com/Expensify/App into issue_27704

5da70bb

Fixing unnecessary server fetch for private notes

ef41837

renaming private notes undefined variable

07508c7

Merge branch 'main' of https://github.com/Expensify/App into issue_27704

4b96037

Fixing new lint errors

b131ffb

MonilBhavsar reviewed Nov 6, 2023

View reviewed changes

MonilBhavsar approved these changes Nov 6, 2023

View reviewed changes

MonilBhavsar merged commit b852d71 into Expensify:main Nov 6, 2023

OSBotify mentioned this pull request Nov 6, 2023

Deploy Checklist: New Expensify 2023-11-06 #30929

Closed

OSBotify mentioned this pull request Nov 8, 2023

Deploy Checklist: New Expensify 2023-11-08 #31077

Closed

		function WithReportAndPrivateNotesOrNotFound(props) {
		const {route, report, network, session} = props;

	export default function (WrappedComponent) {
	const propTypes = {
	/** The HOC takes an optional ref as a prop and passes it as a ref to the wrapped component.
	* That way, if a ref is passed to a component wrapped in the HOC, the ref is a reference to the wrapped component, not the HOC. */
	forwardedRef: PropTypes.func,

	/** The report currently being looked at */
	report: reportPropTypes,

	/** The report metadata */
	reportMetadata: reportMetadataPropTypes,

	/** Array of report actions for this report */
	reportActions: PropTypes.shape(reportActionPropTypes),

	/** The policies which the user has access to */
	policies: PropTypes.objectOf(
	PropTypes.shape({
	/** The policy name */
	name: PropTypes.string,

	/** The type of the policy */
	type: PropTypes.string,
	}),
	),

	/** Route params */
	route: PropTypes.shape({
	params: PropTypes.shape({
	/** Report ID passed via route */
	reportID: PropTypes.string,

	/** ReportActionID passed via route */
	reportActionID: PropTypes.string,
	}),
	}).isRequired,

	/** Beta features list */
	betas: PropTypes.arrayOf(PropTypes.string),

	/** Indicated whether the report data is loading */
	isLoadingReportData: PropTypes.bool,

	/** Is the window width narrow, like on a mobile device? */
	isSmallScreenWidth: PropTypes.bool.isRequired,
	};

	const defaultProps = {
	forwardedRef: () => {},
	reportActions: {},
	report: {},
	reportMetadata: {
	isLoadingReportActions: false,
	isLoadingMoreReportActions: false,
	},
	policies: {},
	betas: [],
	isLoadingReportData: true,
	};

	// eslint-disable-next-line rulesdir/no-negated-variables
	function WithReportAndReportActionOrNotFound(props) {
	const getReportAction = useCallback(() => {

	const shouldShowFullScreenLoadingIndicator = isLoadingPrivateNotes !== false && isPrivateNotesEmpty;
	const shouldShowFullScreenLoadingIndicator = isLoadingPrivateNotes && isPrivateNotesEmpty;

	if (isLoadingPrivateNotes !== false \|\| !isPrivateNotesEmpty) {
	if (isLoadingPrivateNotes \|\| !isPrivateNotesEmpty) {

Fix for Not found page appearing for private notes list, view and edit pages in deep link #29636

Fix for Not found page appearing for private notes list, view and edit pages in deep link #29636

Conversation

salonikumawat28 commented Oct 15, 2023

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Oct 15, 2023

github-actions bot commented Oct 15, 2023 • edited Loading

salonikumawat28 commented Oct 15, 2023

salonikumawat28 commented Oct 15, 2023

salonikumawat28 commented Oct 16, 2023

ArekChr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonikumawat28 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

ArekChr Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

salonikumawat28 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonikumawat28 Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonikumawat28 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonikumawat28 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

ArekChr Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

salonikumawat28 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonikumawat28 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

ArekChr commented Oct 19, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

ArekChr commented Oct 19, 2023

salonikumawat28 commented Oct 19, 2023

ArekChr commented Oct 20, 2023

ArekChr commented Oct 20, 2023

Choose a reason for hiding this comment

salonikumawat28 Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

salonikumawat28 commented Oct 20, 2023

salonikumawat28 commented Oct 23, 2023

ArekChr left a comment

Choose a reason for hiding this comment

salonikumawat28 commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonikumawat28 Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonikumawat28 commented Oct 25, 2023

salonikumawat28 commented Oct 25, 2023

MonilBhavsar commented Oct 25, 2023

MonilBhavsar left a comment

Choose a reason for hiding this comment

salonikumawat28 commented Oct 26, 2023

MonilBhavsar commented Oct 26, 2023

MonilBhavsar commented Oct 26, 2023

github-actions bot commented Oct 15, 2023 •

edited

Loading

salonikumawat28 Oct 18, 2023 •

edited

Loading

ArekChr Oct 18, 2023 •

edited

Loading

salonikumawat28 Oct 18, 2023 •

edited

Loading

salonikumawat28 Oct 19, 2023 •

edited

Loading

salonikumawat28 Oct 18, 2023 •

edited

Loading

salonikumawat28 Oct 18, 2023 •

edited

Loading

ArekChr Oct 18, 2023 •

edited

Loading

salonikumawat28 Oct 18, 2023 •

edited

Loading

salonikumawat28 Oct 18, 2023 •

edited

Loading

ArekChr commented Oct 19, 2023 •

edited

Loading

salonikumawat28 Oct 20, 2023 •

edited

Loading

salonikumawat28 commented Oct 25, 2023 •

edited

Loading

salonikumawat28 Oct 25, 2023 •

edited

Loading

salonikumawat28 commented Nov 6, 2023 •

edited

Loading

salonikumawat28 Nov 6, 2023 •

edited

Loading