Skip to content

[NoQA] add generatePolicyID - #10565

Merged
thienlnam merged 4 commits into
mainfrom
marco-generatePolicyID
Aug 25, 2022
Merged

[NoQA] add generatePolicyID#10565
thienlnam merged 4 commits into
mainfrom
marco-generatePolicyID

Conversation

@marcochavezf

@marcochavezf marcochavezf commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

@thienlnam thienlnam left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/libs/actions/Policy.js Outdated
@thienlnam

Copy link
Copy Markdown
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

@thienlnam thienlnam left a comment

Copy link
Copy Markdown
Contributor

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

thienlnam commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

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

melvin-bot Bot commented Aug 25, 2022

Copy link
Copy Markdown

@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
Copy Markdown
Contributor

Tests had passed

@OSBotify

Copy link
Copy Markdown
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
Copy Markdown
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