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

Updated logic to Not allow future date as incorporation date #5939

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

PrashantMangukiya
Copy link
Contributor

@PrashantMangukiya PrashantMangukiya commented Oct 18, 2021

@puneetlath PR is ready for review.

Details

Updated logic to show error whenever future date entered as incorporation date.
So now it will not allow to proceed if future date entered as company incorporation date.
Proposal: #5722 (comment)
Confirmation: #5722 (comment)

Fixed Issues

$ #5722

Tests | QA Steps

  1. Launch the app
  2. Log in with expensifail account
  3. Enable Staging server
  4. Click on deeplink https://staging.new.expensify.com/bank-account/contract
  5. Log in with Chase credentials
  6. In Company Information Page put future date as Incorporation Date.
    For example 01.01.2022 or Any date greater than current date.
  7. Click Save & Continue.
    It will show error message "Please enter a valid date" under incorporation date if future date entered.
    So It will not allow to proceed if future date entered as incorporation date.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Web.mp4

Mobile Web

MobileWeb.mov

Desktop

Desktop.mp4

iOS

iOS.mov

Android

Android.mp4

@PrashantMangukiya PrashantMangukiya requested a review from a team as a code owner October 18, 2021 23:47
@MelvinBot MelvinBot requested review from puneetlath and removed request for a team October 18, 2021 23:47
i.e. Displayed a more specific error message if incorporation date is in the future. e.g. "Incorporation date cannot be in the future."
@PrashantMangukiya
Copy link
Contributor Author

@puneetlath can you please review the pr again, I also updated screen record with latest implementation. It shows one file conflict, I think it can be resolved before merge. Please let me know if anything need from my side. Thanks.

@puneetlath
Copy link
Contributor

@PrashantMangukiya I'll give it another review! Can you please go ahead and resolve the merge conflict?

puneetlath
puneetlath previously approved these changes Nov 2, 2021
@PrashantMangukiya
Copy link
Contributor Author

@puneetlath thanks,

this pull request shows me this message Only those with write access to this repository can merge pull requests. It looks like I have no write access to this repo. Can you please guide me if any way to resolve conflict from my side ? I will be happy to do it.
Screenshot 2021-11-02 at 10 27 58 PM

@puneetlath
Copy link
Contributor

What do you see when you click the "resolve conflicts" button?

@PrashantMangukiya
Copy link
Contributor Author

@puneetlath thanks for helping. It allowed me to edit code and resolve when clicked Resolve Conflicts button. I just resolved it. Let us wait to finish all checks.

@puneetlath
Copy link
Contributor

Happy to help!

@PrashantMangukiya
Copy link
Contributor Author

@Luke9389 Updated clearDateErrorsAndSetValue() code as per your request, also pushed new commit. You can review it. Thanks.

puneetlath
puneetlath previously approved these changes Nov 3, 2021
Luke9389
Luke9389 previously approved these changes Nov 4, 2021
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes 👍
I ask every contributor this one last question after reviews are over. Have you tested this on all platforms after the most recent change? This helps us reduce regressions and often ends up in the contributor being paid faster.

Also, we have a [HOLD] right now due to a company retreat, so we need to wait on merging this.

@Luke9389 Luke9389 changed the title Updated logic to Not allow future date as incorporation date [HOLD] Updated logic to Not allow future date as incorporation date Nov 4, 2021
@PrashantMangukiya
Copy link
Contributor Author

Yes, I did testing locally before every commit I pushed to pr, so you can freely merge it once hold released.

@mallenexpensify
Copy link
Contributor

Removed Company Offsite HOLD, feel free to merge @puneetlath

@puneetlath puneetlath changed the title [HOLD] Updated logic to Not allow future date as incorporation date Updated logic to Not allow future date as incorporation date Nov 16, 2021
@puneetlath puneetlath merged commit fdcaf5d into Expensify:main Nov 16, 2021
@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 @puneetlath in version: 1.1.15-18 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

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.

6 participants