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.

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

[No QA] Change reportID type from number to string #11362

Merged
merged 18 commits into from
Sep 29, 2022

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Sep 27, 2022

Fixed Issues

Part of https://github.com/Expensify/Expensify/issues/227425

Tests

  1. Select a chat in the LHN
  2. Verify that the row in the LHN shows that the chat is selected (it has a slightly darker background than the rest)

There are a few propType warnings that happen regarding the IOU chat reportIDs, but that API is not fully migrated over to an OfflineFirst world, so those will be fixed later when it's migrated.

  • Verify that no errors appear in the JS console

QA Steps

  1. Select a chat in the LHN
  2. Verify that the row in the LHN shows that the chat is selected (it has a slightly darker background than the rest)
  • Verify that no errors appear in the JS console

@tgolen tgolen self-assigned this Sep 27, 2022
@tgolen tgolen changed the title [No QA] Change reportID type from number to string [HOLD Web-Expensify#34976] [No QA] Change reportID type from number to string Sep 28, 2022
@tgolen tgolen marked this pull request as ready for review September 28, 2022 14:53
@tgolen tgolen requested a review from a team as a code owner September 28, 2022 14:53
@melvin-bot melvin-bot bot requested review from danieldoglas and removed request for a team September 28, 2022 14:54
@tgolen tgolen changed the title [HOLD Web-Expensify#34976] [No QA] Change reportID type from number to string [No QA] Change reportID type from number to string Sep 29, 2022
Copy link
Contributor

@danieldoglas danieldoglas left a comment

Choose a reason for hiding this comment

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

Tests are failing. Overall changes look good to me, but I added a question that might be mostly because I don't fully understand props on ReactNative.

reportName: PropTypes.string,

/** ID of the report */
reportID: PropTypes.string.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: since reportID is required here, won't it break the usages where reportID was not required (or even existed) in the props where it was not necessary? files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so I ended up removing the .isRequired here, however, I not sure what you mean by "files". A report cannot exist in Onyx without a reportID since the collection is keyed by reportID (but not all reports going to the props comes straight from Onyx either).

Copy link
Contributor

@danieldoglas danieldoglas Sep 29, 2022

Choose a reason for hiding this comment

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

I mean other JS files like src/pages/home/report/ReportActionsList.js, that was not requiring reportID.

But I think removing isRequired solves it! Thanks!

Copy link
Contributor

@danieldoglas danieldoglas left a comment

Choose a reason for hiding this comment

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

LGTM

@danieldoglas danieldoglas merged commit 1ff9206 into main Sep 29, 2022
@danieldoglas danieldoglas deleted the tgolen-reportidstrings-0 branch September 29, 2022 22:16
@melvin-bot melvin-bot bot added the Emergency label Sep 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2022

@danieldoglas looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@danieldoglas
Copy link
Contributor

not emergency

@OSBotify
Copy link
Contributor

✋ 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
Copy link
Contributor

🚀 Deployed to staging by @danieldoglas in version: 1.2.11-0 🚀

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

@aldo-expensify
Copy link
Contributor

I think this PR may have missed these places:

  • Shouldn't the client generated reportIDs be strings too?

    App/src/libs/ReportUtils.js

    Lines 577 to 579 in 87921ca

    function generateReportID() {
    return (Math.floor(Math.random() * (2 ** 21)) * (2 ** 32)) + Math.floor(Math.random() * (2 ** 32));
    }

reportID: PropTypes.number.isRequired,

There are some iouReportID and chatReportID as numbers, but from the PR description I'm getting that these were intentionally skipped, is that correct?

@tgolen
Copy link
Contributor Author

tgolen commented Oct 5, 2022

Ah, I did totally miss gernerateReportID(). Good catch. Do you want to do a PR on that?

I did intentionally skip the others, yes, since it looked like those APIs had not been migrated over to NewDot APIs yet in PHP. I didn't want to make any changes on the old API endpoints.

@aldo-expensify
Copy link
Contributor

Ok! I'll do it

@aldo-expensify aldo-expensify mentioned this pull request Oct 5, 2022
93 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Oct 5, 2022

🚀 Deployed to production by @AndrewGable in version: 1.2.11-4 🚀

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

smrutiparida pushed a commit to autosave-app/App that referenced this pull request Oct 13, 2022
…ngs-0

[No QA] Change reportID type from number to string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants