-
Notifications
You must be signed in to change notification settings - Fork 2
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
Processor initial config and Create Payment authorisation API implementation #3
Conversation
…-implementation # Conflicts: # processor/package-lock.json # processor/package.json
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
const handler = statusHandler({ | ||
timeout: 3000, | ||
checks: [ | ||
healthCheckCoCoPermissions({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method will need improvement in terms of naming and managing permissions, client credentials might not have that hardcoded scope.
}), | ||
}); | ||
|
||
ctCart = await this.ctCartService.addPayment({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add some logic for error scenarios but something for later.
@@ -17,7 +17,7 @@ | |||
<body> | |||
<div id="app"></div> | |||
<script type="module"> | |||
import { Connector } from '/lib/main.ts'; | |||
import { Connector } from '/enabler/src/main.ts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried running this locally with npm run dev it cant find this file. we will need to change this to :
/src/main.ts
for it to start in dev
@@ -20,5 +20,6 @@ | |||
"noUnusedParameters": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be better to set strict: true so that we can ensure stronger guarantees of program correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree @koonweiteo-commercetools , but if that requiere to spend some time lets first try to deploy so we have a running connector and checkout can continue with the development of the integration and then this can be fixed.
Please change vite config entry to |
enabler/src/public/session.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a new VITE env for session api url or region?
const res = await fetch(`http://localhost:3004/api/${projectKey}/sessions`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koonweiteo-commercetools this file was just for testing purposes and we used it only locally, at some point we will remove this file entirelly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR, I am trying to focus only on Processor implementation, I will create another PR for Enabler changes.
you can ignore the small corrections in Enabler in this PR.
processor/src/config/config.ts
Outdated
@@ -0,0 +1,24 @@ | |||
export const config = { | |||
// Required by Payment SDK | |||
projectKey: process.env.CTP_PROJECT_KEY || 'test', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are required, it will be better to fail at startup instead of setting a default value as it will cause runtime errors.
We can leverage on fastify core plugin: https://github.com/fastify/fastify-env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree... anyway lets do not focus on thee small things at this point and lets make sure to have a running connector and then we can fix the details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@praveenkumarct if it is necessary lets create tickets to follow up on this once we have a running connector.
Added TODOs for the other comments and few other small improvements in the processor implementation. |
Processor implementation:
TODO:
Ref: https://commercetools.atlassian.net/browse/IM-833