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

Add support for Wallet Orders #137

Merged
merged 2 commits into from Feb 15, 2024

Conversation

cjnewbs
Copy link
Contributor

@cjnewbs cjnewbs commented Feb 5, 2024

Summary

Added support for Apple Wallet orders. Thought I'd open a draft PR to get some feedback before I go any further. I have tested that this works by creating a bare minimum test order, and AirDropping it to my iPhone.

  • I was originally going to name the new class PKOrder as mentioned, then I went to find the equivalent for orders but was unable to find one. I then saw the mime type was application/vnd.apple.finance.order so thought FinanceOrder might work? Happy for this to be renamed back to PKOrder,
  • Updated PHPdoc type for $json to string,
  • Removed copyright date from new class so it doesn't get outdated,
  • createSignature: Used the parent method as I don't think the path has any affect on the signature creation,
  • createManifest: I did start by replacing this method with one that uses the hash method like in the FinanceOrder class but realised that the pass also needs an icon.png file so copied it to the child method,
  • getName: Updated to use the const's from the class that was instantiated.

Happy to revert all the changes I made to make the PKPass class a little bit more flexible and instead override the method in FinanceOrder.

Todo:

  • Unit tests. I'm thinking I will need to create a merchant identifier and signing certificate for using with a unit test, unless you guys want to create one yourself.

Will resolve #136

@tschoffelen
Copy link
Member

This looks amazing! It's a lot cleaner than I thought it would be to extend the class like this, great!

Definitely mergeable if we add some docs. A test would be good indeed. I think the signing cert for the current test could be reused for the orders test. The test doesn't have to generate a signature that Apple is happy with/recognises, at least if you're happy using the same type of test setup used for the PKPass test.

@cjnewbs
Copy link
Contributor Author

cjnewbs commented Feb 6, 2024

I have added a unit test (I'm a Magento developer so used an order in the sampledata as an example to base it on). I re-read the original comment on the issue and had another think about making some more changes to add an abstract class, but on reflection I think its probably ok as-is. If Apple add another feature that requires creating a "signed bundle/package" perhaps it'll be worth taking another look then.

For the unit test I originally thought about creating a real Apple one but as you mentioned it only really needs to succeed in signing as there is no integration test validating it on a device. I have used the same data (apart from the order and merchant identifier) to create one I have validated on device so I am happy with that. Marking as ready for review.🙂

@cjnewbs cjnewbs marked this pull request as ready for review February 6, 2024 22:39
@tschoffelen
Copy link
Member

Great, I'm going to get this merged now.

In the future we might want to add some docs to explain the usage of this, but hopefully for now the new test should guide people in the right direction if they stumble upon this.

@tschoffelen tschoffelen merged commit 18175dc into includable:master Feb 15, 2024
8 checks passed
@tschoffelen
Copy link
Member

Released to Packagist as 2.3.0 - thanks again @cjnewbs!

@cjnewbs
Copy link
Contributor Author

cjnewbs commented Feb 18, 2024

Thats amazing, glad you like it 🤩 ! I realised I forgot to add any docs to this earlier last week. Just sat down to write some simple ones and noticed this merged. I'll get a few put together and create a subsequent PR at some point. Hope that's ok 🙂

@tschoffelen
Copy link
Member

Of course, thank you!

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.

Wallet Orders
2 participants