Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed Loading spinner on login form and crash after Login #3865

Merged
merged 4 commits into from
Jul 7, 2021
Merged

Fixed Loading spinner on login form and crash after Login #3865

merged 4 commits into from
Jul 7, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jul 2, 2021 •
edited
Loading

Details

#3838 (comment)
#3838 (comment)

Fixed Issues

$ Fixes #3838

Tests | QA Steps

1. Lunch the app
2. Log in with account A
3. Send some messages and attachments
4. Log out from Account A
5. Log in with Account B

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@parasharrajat parasharrajat changed the title fixed error when draft is null Fixed Loading spinner on login form and crash after Login Jul 4, 2021
@parasharrajat parasharrajat marked this pull request as ready for review July 4, 2021 01:56
@parasharrajat parasharrajat requested a review from a team as a code owner July 4, 2021 01:56
@MelvinBot MelvinBot requested review from chiragsalian and removed request for a team July 4, 2021 01:56
chiragsalian
chiragsalian requested changes Jul 6, 2021
View reviewed changes
src/libs/OptionsListUtils.js Outdated Show resolved Hide resolved
src/libs/OptionsListUtils.js
@@ -169,9 +169,9 @@ function createOption(personalDetailList, report, draftComments, {
}) {
const hasMultipleParticipants = personalDetailList.length > 1;
const personalDetail = personalDetailList[0];
const hasDraftComment = report
const reportDraftComment = report
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why changing this variable from boolean to object has anything to do with your fixing loading spinner? It looks like it's just used as a boolean below anyway. Can you explain?

Copy link
Member Author

@parasharrajat parasharrajat Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So There are two issues, I am trying to fix in this PR. Sometimes, the lodashGet(draftComments, ${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${report.reportID}, '') returns null. So it will throw an error here that .length is not present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, i see. I didn't expect that value to be null (and hence not use the lodash get default value). Weird. Anyway, thanks for the fix 👍

src/libs/OptionsListUtils.js
@@ -297,13 +297,13 @@ function getOptions(reports, personalDetails, draftComments, activeReportID, {
return;
}

const hasDraftComment = report
const reportDraftComment = report
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment here. I don't get why this variable change is relevant to your PR.

@parasharrajat
Copy link
Member Author

Updated. Thanks

chiragsalian
chiragsalian approved these changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM

src/libs/OptionsListUtils.js
@@ -169,9 +169,9 @@ function createOption(personalDetailList, report, draftComments, {
}) {
const hasMultipleParticipants = personalDetailList.length > 1;
const personalDetail = personalDetailList[0];
const hasDraftComment = report
const reportDraftComment = report
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, i see. I didn't expect that value to be null (and hence not use the lodash get default value). Weird. Anyway, thanks for the fix 👍

@chiragsalian chiragsalian merged commit 38830cf into Expensify:main Jul 7, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Jul 7, 2021

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

@kidroca
Copy link
Contributor

kidroca commented Jul 7, 2021 •
edited
Loading

@parasharrajat
I've stumbled on an issue, added a comment here: 6be5a34#r664763031

you don't need the !! here right. You could just do,

reportDraftComment && reportDraftComment.length > 0

This is not true and completely broke the app for me 😃

image

src/pages/home/sidebar/OptionRow.js

                            {option.hasDraftComment && (
                                <View style={styles.ml2}>
                                    <Icon src={Pencil} height={16} width={16} />
                                </View>
                            )}

I have some empty "" draft comment somewhere and it just crashes the app

The cast to boolean is necessary, though I would go with the explicit

Boolean(reportDraftComment) && reportDraftComment.length > 0

or

Boolean(reportDraftComment && reportDraftComment.length > 0)

I don't know why I have an empty comment but it seems plausible and since reportDraftComment is a string it should be casted to prevent accidents like this

@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 7, 2021 •
edited
Loading

@kidroca Thanks for notifying me. That's why I added the boolean conversion but couldn't think of it while review. I will patch this up in a small PR.

@parasharrajat parasharrajat mentioned this pull request Jul 7, 2021
Merged
@chiragsalian
Copy link
Contributor

Oh crap, nice catch @kidroca. I didn't consider the weirdness around empty string in JS wrt boolean operations.
Thanks for following up on the fixes @parasharrajat.

@OSBotify OSBotify mentioned this pull request Jul 7, 2021
Closed
57 tasks
@roryabraham
Copy link
Contributor

Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kidroca kidroca kidroca left review comments

@chiragsalian chiragsalian chiragsalian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[HOLD - for payment on July 14] Chat - Loading spinner appears after log in with different account
5 participants
@parasharrajat @OSBotify @kidroca @chiragsalian @roryabraham