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

Refactor Apple Wallet code (#76) #91

Merged

Conversation

aahna-ashina
Copy link
Member

@aahna-ashina aahna-ashina commented Oct 13, 2022

Dework Task

https://app.dework.xyz/nation3/passport-services-p?taskId=ae6efa25-b222-4700-afc8-63e65b870cb9

Related GitHub Issue

#76

Improve stability of APNs integration:

  • 36f5e8d Improve asynchronous calls to APNs in Passes.ts, so that a JSON response is not returned until all notification requests have been confirmed by APNs.

How Has This Been Tested?

  • Status checks pass
  • 138492a Mock APN provider during unit tests
  • Increased code coverage from 80% to 85%
  • Works on Goerli
  • Works on Mainnet

Are There Admin Tasks?

Nada

@vercel
Copy link

vercel bot commented Oct 13, 2022

@aahna-ashina is attempting to deploy a commit to the Nation3 Team on Vercel.

A member of the Team first needs to authorize it.

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #91 (a62f5a1) into main (2182909) will increase coverage by 5.08%.
The diff coverage is 73.07%.

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   80.88%   85.96%   +5.08%     
==========================================
  Files           5        7       +2     
  Lines         204      228      +24     
  Branches       23       26       +3     
==========================================
+ Hits          165      196      +31     
+ Misses         38       30       -8     
- Partials        1        2       +1     
Impacted Files Coverage Δ
server/utils/APNProvider.ts 0.00% <0.00%> (ø)
server/utils/Passes.ts 94.40% <89.28%> (+14.40%) ⬆️
server/utils/DateUtils.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aahna-ashina aahna-ashina requested review from luisivan and xPi2 October 14, 2022 10:14
@aahna-ashina aahna-ashina marked this pull request as ready for review October 19, 2022 09:07
@aahna-ashina
Copy link
Member Author

aahna-ashina commented Oct 19, 2022

@luisivan This PR improves calls to /api/pushLastUpdate by returning a more detailed JSON response:

{
    "summary": "1 sent, 3 failed",
    "sent": [ ... ],
    "failed": [ ... ]
}

Also the code is changed from async to await, so that a response is not returned until all push notification requests have been sent to Apple. For now, the waiting time is just a few seconds. And in the future, with hundreds/thousands of registered passes, we should improve the user experience by building a UI where you can see a progress bar while sending out pass update notifications and waiting for them to complete.

@@ -4,7 +4,7 @@ module.exports = {
testEnvironment: 'node',
coverageThreshold: {
global: {
lines: 80.00
lines: 79.00
Copy link
Contributor

Choose a reason for hiding this comment

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

😄

Copy link
Member Author

Choose a reason for hiding this comment

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

How sad 🥲

This reverts commit ff8cd3b, reversing
changes made to eaa5568.
test(apple): debug template version 4

debug: add square icon

debug: restore original logo and strip

Revert "debug: restore original logo and strip"

This reverts commit 47d4345.

Revert "debug: add square icon"

This reverts commit bff7d91.

Revert "test(apple): debug template version 4"

This reverts commit 5bff363.

Revert "chore(apple): add template version 4 (nation3#76)"

This reverts commit f327105.
@aahna-ashina aahna-ashina force-pushed the dw-724/refactor-apple-wallet-code branch from 5547aa7 to 09ab583 Compare October 20, 2022 07:58
@aahna-ashina
Copy link
Member Author

@luisivan @okhaimie-dev @xPi2 Need your review before merging 🙏

@aahna-ashina
Copy link
Member Author

@johnmark13 Added mocking: 138492a

Copy link
Contributor

@xPi2 xPi2 left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe I'm missing enough context to give better feedback.

Copy link
Contributor

@johnmark13 johnmark13 left a comment

Choose a reason for hiding this comment

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

I think this is really clever 💪

@@ -4,7 +4,7 @@ module.exports = {
testEnvironment: 'node',
coverageThreshold: {
global: {
lines: 80.00
lines: 85.00
Copy link
Contributor

Choose a reason for hiding this comment

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

💪

@aahna-ashina aahna-ashina merged commit 665c543 into nation3:main Oct 31, 2022
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.

5 participants