-
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
feat: implemented user registration, sending email notifications, and api documentation #9
Conversation
Features
Update
Bug FixesContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
@vickywane This is a static document. We need a CI workflow that automatically re-generates and publishes to github pages API docs based on any PR included code changes. |
@vickywane upon further research, I noticed that the latest best practice is as follows:
Notice that there are some details that need to be carefully worked out. Netlify deployments guarantee that all production deployments remain at their permanent versioned URLs. This is critically important to avoid maintenance windows with service downtime. If a serverless API has a breaking change, the currently deployed PWA will keep running against the previous API version URL. Deploying a new API version and consequently a new PWA version should lead to a smooth user transition to the new versions without any downtime. This cycle has to be carefully thought out and tested. |
@ivelin i have the OpenAPI generation ( client, stubs & docs ) setup. However, I need more clarification on how they would be integrated into this repo. After each generation, it creates an entirely new project with several unneeded files. I feel we should clean up some of the generated files ( using a script) and the rest should be stored in |
Do we still need to publish the API to Github pages? |
The end results should be a freshly updated public API docs that is pushed in sync with code release. |
Makes sense. We only need to commit in github the source files required to re-produce a binary release of a client SDK, server implementation and API docs. Interim local build artifacts can be ignored as usually. |
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.
Nice progress:
TODO:
-
run tests against a locally deployed version of the API in the current PR
-
run tests against a netlify preview deployment of the API
-
Add a code coverage report after the test run. See here.
-
semantic release
- tag and label a new github release
- publish client sdk to npm
-
follow up: PWA import of client sdk for user subscription management
Codecov Report
@@ Coverage Diff @@
## main #9 +/- ##
=======================================
Coverage ? 22.01%
=======================================
Files ? 8
Lines ? 536
Branches ? 157
=======================================
Hits ? 118
Misses ? 418
Partials ? 0 |
Yes, agreed. Only keep in github files that are required for the build to succeed. Interim files can be cleaned up. Deployment artifacts should be automatically tested and published. Docs should be updated and published with each new release. See how we do that for the Edge Python API. Once we have an URL for the premium subscription API, we will included it in the official docs. |
I am not sure what you are asking me here. Please clarify what the issue is and what you want me to help with. |
This has been implemented partly in the |
@vickywane |
@ivelin I had to make adjustments here for code coverage. Previously we decided to stick with using tests from Postman ( run with Newman), but this doesn't fulfill the code coverage requirement. ( see this discussion ). So I had to bring back using Mocha tests. Using Istanbul, we can generate code coverage. I think we later have to decide which to stick with or evaluate if we really need code coverage here. |
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.
@vickywane looks like the main Open API definition file disappeared. Only docs and js files are left in this PR. Other comments inline.
.github/workflows/ci.yml
Outdated
env: | ||
CI: true | ||
STRIPE_TEST_KEY: sk_test_7LEesZrYrgDtyxsc6Hy5i0lS00GVFEbmqb | ||
ENDPOINT: https://deploy-preview-${{github.event.number}}--ambianic-serverless.netlify.app/.netlify/functions |
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.
@vickywane comments on my question from April 6?
@ivelin I have fixed the naming comments on this pull request. I am however being held back by an issue with the In the interim, can I substitute the This is the reason why the current CI check fails and also should address your two comments on the CI file above here. |
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.
See comments. Please point me to your own fork's URL where several CI runs completed successfully with test runs, API docs published and semantic releases published.
|
||
## Documentation for Authorization | ||
|
||
All endpoints do not require authorization. |
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.
@vickywane How are we going to protect these public APIs from malicious attacks that result into random notifications sent to users?
I have created this new issue within this repository to track work done to address and provide a solution for this potential problem. It would involve the PWA and edge device authenticating with this Cloud API each time a request is made. |
* feat: project CI * feat: corrected docs and added architecture diagram to readme * feat: project CI * update: added code coverage report * update: added code coverage report * update: updated function to use local netlify server * update: updated function to use local netlify server * fix: moved stripe key to secrets && improved scripts * fix: update * fix: update Co-authored-by: Ivelin Ivanov <[email protected]>
@ivelin As requested, below are the relevant links generated from my own fork of this pull request here;
Below is a list of follow-up issues generated from this feature to worked upon after this pull request is merged; |
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.
A couple unresolved comments.
|
||
## Documentation for Authorization | ||
|
||
All endpoints do not require authorization. |
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.
@vickywane this comment is still unresolved. What protection do we have against malicious use of these public APIs?
openapi-docs/docs/DefaultApi.md
Outdated
[**deleteSubscription**](DefaultApi.md#deleteSubscription) | **DELETE** /subscription | Delete an Ambianic's user subscription | ||
[**getSubscriptionData**](DefaultApi.md#getSubscriptionData) | **GET** /subscription | Get a user's subscription data | ||
[**sendNotification**](DefaultApi.md#sendNotification) | **POST** /notification | Send an event detection notification | ||
[**subscribeUser**](DefaultApi.md#subscribeUser) | **POST** /subscribe | Subscribe Ambianic user to premium notification |
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.
@vickywane for naming and semantic consistency, we should consider using
POST /subscription
as an endpoint to add a new subscription for a user.
instead of a separate endpoint name
POST /subscribe
That way we end up with two clean end points
/subscription
that handles all CRUD operations (created, read, update, delete) for user subscriptions.
and
/notification
for triggering premium notifications.
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.
@vickywane this comment is still unresolved. What protection do we have against the malicious use of these public APIs?
By default, Netlify doesn't provide any means of protection for the deployed endpoints. However, I have created a follow-up issue ( here ) to implement protection of the endpoints by ensuring that each request received is passed through a middleware that checks the origin of the request using a similar approach from Middlyjs.
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.
Okay.
Something to note is that each time a change is made in the route, it also has to be updated in the Ambianic PWA.
@ivelin As requested, below is a list of objectives for this pull request. Each respective item below contains a link to where they were implemented in the continuous integration workflow and executed on my fork;
|
Correct. Which is why we need to be thoughtful before we make it official.
…On Tue, May 25, 2021 at 5:55 PM Nwani Victory ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In openapi-docs/docs/DefaultApi.md
<#9 (comment)>
:
> @@ -0,0 +1,224 @@
+# AmbianicFunctionsCollection.DefaultApi
+
+All URIs are relative to *https://33743be6-3197-4dd4-8471-50b3640320c9.mock.pstmn.io*
+
+Method | HTTP request | Description
+------------- | ------------- | -------------
+[**deleteSubscription**](DefaultApi.md#deleteSubscription) | **DELETE** /subscription | Delete an Ambianic's user subscription
+[**getSubscriptionData**](DefaultApi.md#getSubscriptionData) | **GET** /subscription | Get a user's subscription data
+[**sendNotification**](DefaultApi.md#sendNotification) | **POST** /notification | Send an event detection notification
+[**subscribeUser**](DefaultApi.md#subscribeUser) | **POST** /subscribe | Subscribe Ambianic user to premium notification
Okay.
Something to note is that each time a change is made in the route, it also
has to be updated in the Ambianic PWA.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARBUFNFXCV26UVQXKWHDFLTPQTFLANCNFSM4YZGSWBA>
.
|
@ivelin I just implemented a change to use a local mock server created using With this new change, you would find some |
That's great progress.
…On Wed, May 26, 2021 at 5:37 PM Nwani Victory ***@***.***> wrote:
@ivelin <https://github.com/ivelin> I just implemented a change to use a
local mock server created using @stoplight/prism-cli for mocking the
responses made in each of the API tests. Previously we used mock servers
managed by postman.
With this new change, you would find some inlineResponse files
autogenerated from the openapi-spec.yaml file. These files contain the
data for the mock response served by Prism
<https://meta.stoplight.io/docs/prism/README.md>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARBUFMYBTP5HVHYD5V5F4LTPVZZTANCNFSM4YZGSWBA>
.
|
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.
See inline comments.
If adding all generated docs for the Cloud API is what is left from this pull request, can we proceed with a merge so I can add the live links and push it to the docs pull request? |
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.
Looks good. Time to merge.
This resolves this issue
This PR contains a YAML exported API Definition for the Ambianic-functions-API using the created API collection on Postman.