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

Documentation for Notification Module #45

Merged
merged 35 commits into from
Apr 15, 2019
Merged

Conversation

LEQADA
Copy link
Contributor

@LEQADA LEQADA commented Apr 2, 2019

No description provided.

docs/BEST_PRACTICES.md Outdated Show resolved Hide resolved
docs/BEST_PRACTICES.md Outdated Show resolved Hide resolved
docs/BEST_PRACTICES.md Outdated Show resolved Hide resolved
with at least 2 running instances at the same time in order to minimize downtime
possibility.

It's also recommended to use HTTPS.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe lets make bullet point out of every best practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

notification/docs/IntegrationGuide.md Outdated Show resolved Hide resolved
notification/docs/IntegrationGuide.md Outdated Show resolved Hide resolved
notification/docs/IntegrationGuide.md Outdated Show resolved Hide resolved
notification/docs/IntegrationGuide.md Outdated Show resolved Hide resolved
you have to register it in the Adyen Customer Area in order to receive notifications.

## Register the endpoint
1. Go to your Adyen Customer Area.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Link to the Customer Area would be very friendly :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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


There are 2 different types of tests. Don't forget to provide all required environmental variables:
1. [Unit tests](../test/unit) - these tests are mocking all external communications.
1. [Integration tests](../test/integration) - these tests interacts with real 3rd party systems.
Copy link
Collaborator

Choose a reason for hiding this comment

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

A possibility to IT and unit test separately would be nice.

Copy link
Contributor Author

@LEQADA LEQADA Apr 8, 2019

Choose a reason for hiding this comment

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

Created an issue #47

Copy link
Contributor

Choose a reason for hiding this comment

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

Only CTP is real integration. Adyen notification is still a mock. Could be a bit confusing for new devs. You should mention it.

Copy link
Contributor Author

@LEQADA LEQADA Apr 10, 2019

Choose a reason for hiding this comment

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

@lojzatran I removed this explanation. I'll assume that a developer knows the difference between unit and integration tests.

@butenkor
Copy link
Collaborator

butenkor commented Apr 5, 2019

Please include in deployment guide an info where docker container can be pulled.

PORT | port on which the application will run | NO | 443

Check out the deployment [Best Practices documentation](../../docs/BEST_PRACTICES.md)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here should be point called Deployment which describes which docker container is to be used and how to start it.

@@ -0,0 +1,67 @@
# Integration of payment into checkout process
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should consist out of two main sections: Deployment (Docker, how to run/setup, env), Configuration (Adyen subscription)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@ahmetoz ahmetoz left a comment

Choose a reason for hiding this comment

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

👍 good job in general but I've some comments and questions.

notification/docs/DevelopmentGuide.md Show resolved Hide resolved
docs/BEST_PRACTICES.md Show resolved Hide resolved
notification/docs/DevelopmentGuide.md Outdated Show resolved Hide resolved
notification/docs/DevelopmentGuide.md Outdated Show resolved Hide resolved
LOG_LEVEL | bunyan log level (`trace`, `debug`, `info`, `warn`, `error`, `fatal`)| NO | `info`
PORT | port on which the application will run | NO | 443

After setting all variables, execute command `npm run start` to run the module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
After setting all variables, execute command `npm run start` to run the module.
After setting all environment variables, execute command `npm run start` to run the module.

notification/docs/IntegrationGuide.md Outdated Show resolved Hide resolved
notification/docs/IntegrationGuide.md Show resolved Hide resolved
notification/docs/IntegrationGuide.md Outdated Show resolved Hide resolved
notification/docs/IntegrationGuide.md Show resolved Hide resolved
notification/docs/IntegrationGuide.md Outdated Show resolved Hide resolved
@LEQADA LEQADA marked this pull request as ready for review April 10, 2019 13:20
- If you accidentally created a subscription you can edit it and uncheck the **Active** checkbox so Adyen doesn't
send there notifications. Then you can contact the Adyen support and ask them to remove the subscription.
- Adyen will queue notifications when the notification service was not reachable or it didn't return a success message
and will try to send it later.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you also write sth about how you save the notifications, what the fields means and how they can use it? Also that you don't map some notifications and what happens in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a general readme
19d1373

LEQADA and others added 2 commits April 11, 2019 10:35
… documentation-notification

# Conflicts:
#	notification/docs/IntegrationGuide.md
notification/docs/DevelopmentGuide.md Outdated Show resolved Hide resolved
notification/docs/DevelopmentGuide.md Outdated Show resolved Hide resolved
LEQADA and others added 2 commits April 15, 2019 18:22
Co-Authored-By: ahmetoz <bilmuhahmet@gmail.com>
@LEQADA LEQADA merged commit 3698144 into master Apr 15, 2019
@LEQADA LEQADA deleted the documentation-notification branch April 15, 2019 16:27
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.

None yet

4 participants