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

[NoQA] add generatePolicyID #10565

Merged
merged 4 commits into from
Aug 25, 2022
Merged

[NoQA] add generatePolicyID #10565

merged 4 commits into from
Aug 25, 2022

Conversation

marcochavezf
Copy link
Contributor

@marcochavezf marcochavezf commented Aug 25, 2022

cc @thienlnam

Details

Add generatePolicyID() to generate PolicyID on the client.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/224679

Tests

N/A This function is going to be used for https://github.com/Expensify/Expensify/issues/224690

QA Steps

N/A

Screenshots

N/A

@marcochavezf marcochavezf requested a review from a team as a code owner August 25, 2022 14:19
@marcochavezf marcochavezf self-assigned this Aug 25, 2022
@melvin-bot melvin-bot bot requested review from MonilBhavsar and removed request for a team August 25, 2022 14:19
@marcochavezf
Copy link
Contributor Author

Hmm it's failing because of no-unused-vars eslint rule, I think it will be better to move this function to NumberUtils.js and import it into Policy.js when it's used. What do you think @thienlnam? In that way, I could also add a unit test here

Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

Looking good! Just one nab about the comment

@thienlnam
Copy link
Contributor

Hmm it's failing because of no-unused-vars eslint rule,

You should just be able to include the variable in the export below to fix this error

@marcochavezf marcochavezf requested a review from thienlnam August 25, 2022 14:38
@marcochavezf marcochavezf dismissed thienlnam’s stale review August 25, 2022 14:38

requested change addressed

Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

looks great, thanks for the ⚡ !

@thienlnam
Copy link
Contributor

thienlnam commented Aug 25, 2022

Going to merge to push this forward and since it's a harmless change - feel free to leave comments still if you notice something @MonilBhavsar

@thienlnam thienlnam merged commit 897ffbc into main Aug 25, 2022
@thienlnam thienlnam deleted the marco-generatePolicyID branch August 25, 2022 15:56
@melvin-bot melvin-bot bot added the Emergency label Aug 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2022

@thienlnam 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.

@thienlnam
Copy link
Contributor

Tests had passed

@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 @thienlnam in version: 1.1.90-0 🚀

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

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.

3 participants