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

Fix/mail #1932

Merged
merged 6 commits into from
Aug 22, 2022
Merged

Fix/mail #1932

merged 6 commits into from
Aug 22, 2022

Conversation

gweiying
Copy link
Contributor

@gweiying gweiying commented Aug 22, 2022

Problem

I broke our Coveralls tests unknowingly (fell below 75%) when I made the changes to include a Postman fallback option (#1912) without adding tests.

Closes #1915 and #1928

Solution

  • Added test routes for email module
  • Replaced axios with cross-fetch
  • Fixed try-catch nesting for OTP generation and sending

Other notes

I would love to propose that we run Coveralls against PRs to develop. Currently, Coveralls runs on PRs to release, but that since merges to releases are usually after we complete feature development and are prepared to cut a stable version, the coverage tests comes a little belated. Seems like Coveralls runs against develop too, my bad.

Tests

@gweiying gweiying changed the base branch from develop to release August 22, 2022 02:55
@gweiying gweiying changed the base branch from release to develop August 22, 2022 04:41
@gweiying gweiying requested review from yong-jie, halfwhole and JasonChong96 and removed request for yong-jie, halfwhole and JasonChong96 August 22, 2022 05:00
@gweiying gweiying marked this pull request as ready for review August 22, 2022 05:00
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

lgtm! yes coveralls has always been there for develop branch as well, just that it normally passes and hence gets drowned in the 10-19 other green ticks in the PR checks 😂

@gweiying gweiying merged commit fe55339 into develop Aug 22, 2022
@gweiying gweiying deleted the fix/mail branch October 7, 2022 07:58
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.

Tech Debt: Remove usage of axios in postman feature
2 participants