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

[#171211343] Adds user-data-processing API #584

Merged
merged 5 commits into from
Mar 4, 2020

Conversation

AleDore
Copy link
Contributor

@AleDore AleDore commented Mar 2, 2020

Adds UserDataProcessing API to let the authenticated user express the will to retrieve or delete her own data (stored into application infrastructure).

@digitalcitizenship
Copy link

digitalcitizenship commented Mar 2, 2020

Affected stories

  • 🌟 #171211343: Come CIT voglio poter esprimere la volontà di veder eliminati i miei dati o ricevere tutti quelli memorizzati nel sistema

Generated by 🚫 dangerJS

@gunzip gunzip requested a review from BurnedMarshal March 2, 2020 13:30
@gunzip gunzip changed the title [#171211343] - user-data-processing-API exposure [#171211343] Adds user-data-processing API Mar 2, 2020
@gunzip
Copy link
Contributor

gunzip commented Mar 2, 2020

can you please fix the tests in userDataProcessingService.test.ts ?

Cannot find module 'generated/backend/SpidLevel' from 'userDataProcessingService.test.ts'

api_backend.yaml Outdated
post:
operationId: upsertUserDataProcessing
summary: Set User's data processing choices
description: Upsert user data processing for the current authenticated user.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change this with the actual use case (something like "Let the authenticated user express his will to retrieve or delete his stored data"

api_backend.yaml Outdated
$ref: "#/definitions/ProblemJson"
"/user-data-processing/{choice}":
x-swagger-router-controller: UserDataProcessingController
post:
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this is a get (not a post)

api_backend.yaml Outdated
post:
operationId: getUserDataProcessing
summary: Get User's data processing
description: Get user data processing for the current authenticated user and the given choice.
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above (more friendly description)

api_backend.yaml Outdated
description: User data processing retrieved
schema:
$ref: "#/definitions/UserDataProcessing"
"400":
Copy link
Contributor

Choose a reason for hiding this comment

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

in which case does this return 400 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory this doesn't return 400 in any case, because backend matches session_token with the fiscalCode stored in Redis Cache, but on the other side io-functions-app provides 400 as a return code. Do i have to remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes if the method cannot return 400

description: Not found.
schema:
$ref: "#/definitions/ProblemJson"
"429":
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not clear on why this may return 429 and upsertUserDataProcessing does not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's my fault, this may return 429 as GetUserDataProcessing ( IResponseErrorTooManyRequests is provided by the controller's method )

jest.clearAllMocks();
});

it("calls the getUserDataProcessing on the UserDataProcessingService with valid values", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

test descriptions must begin with "should .." and describe the expectation. ie.

it("should return a successful response in case a valid payload is provided")

same for every other description.

@@ -0,0 +1,76 @@
/**
* This controller handles reading the user profile from the
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this description is outdated
(take care when cut&pasting pls : )

@codecov-io
Copy link

codecov-io commented Mar 2, 2020

Codecov Report

Merging #584 into master will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
+ Coverage   82.82%   82.92%   +0.10%     
==========================================
  Files          46       48       +2     
  Lines        1339     1394      +55     
  Branches      235      242       +7     
==========================================
+ Hits         1109     1156      +47     
- Misses        219      226       +7     
- Partials       11       12       +1     
Impacted Files Coverage Δ
src/services/userDataProcessingService.ts 96.42% <0.00%> (ø)
src/controllers/userDataProcessingController.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a025cec...9424f5b. Read the comment docs.

api_backend.yaml Outdated
required: true
responses:
'200':
description: User Data processing created.
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
description: User Data processing created.
description: User Data processing created / updated.

api_backend.yaml Outdated
description: User data processing retrieved
schema:
$ref: "#/definitions/UserDataProcessing"
"400":
Copy link
Contributor

Choose a reason for hiding this comment

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

yes if the method cannot return 400

expect(res.kind).toEqual("IResponseErrorInternal");
});

it("returns a 500 response if the response from the getUserDataProcessing API returns something wrong", async () => {
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
it("returns a 500 response if the response from the getUserDataProcessing API returns something wrong", async () => {
it("should return an error if getUserDataProcessing API returns invalid data", async () => {

});
});

it("returns an 429 HTTP error from getUserDataProcessing upstream API", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should...

expect(res.kind).toEqual("IResponseErrorTooManyRequests");
});

it("returns an error if the getUserDataProcessing API returns an error", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

});
});

it("should return a BadRequestError response by calling upsertUserDataProcessing on the UserDataProcessingService with empty user and valid upsert user", async () => {
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
it("should return a BadRequestError response by calling upsertUserDataProcessing on the UserDataProcessingService with empty user and valid upsert user", async () => {
it("should return a bad request response if request user is empty in the request", async () => {

the fact that we're calling UserDataProcessingController#upsertUserDataProcessing is inferred by the describe call

expect(res.kind).toEqual("IResponseErrorTooManyRequests");
});

it("returns an error if the upsertUserDataProcessing API returns an error", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should..

beforeEach(() => {
jest.clearAllMocks();
});
it("returns an upserted user data processing from the API", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should..

@gunzip
Copy link
Contributor

gunzip commented Mar 3, 2020

lgtm. @BurnedMarshal can you please take a look at this PR ?

@gunzip gunzip merged commit 994364e into master Mar 4, 2020
@gunzip gunzip deleted the 171211343-user-data-processing-API branch March 4, 2020 11:06
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.

4 participants