[#175395536] Add Organization logo's upload #83

AleDore · 2020-10-26T12:14:09Z

This PR adds a new endpoint for UploadOrganizationLogooperation. See io-functions-admin PR for further informations

codecov-io · 2020-10-26T12:15:18Z

Codecov Report

Merging #83 into master will decrease coverage by 1.31%.
The diff coverage is 88.63%.

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   85.71%   84.39%   -1.32%     
==========================================
  Files          36       21      -15     
  Lines         728      673      -55     
  Branches       73       63      -10     
==========================================
- Hits          624      568      -56     
- Misses        100      104       +4     
+ Partials        4        1       -3

Impacted Files	Coverage Δ
UploadOrganizationLogo/handler.ts	`82.75% <82.75%> (ø)`
CreateService/handler.ts	`86.84% <100.00%> (+1.54%)`	⬆️
UpdateService/handler.ts	`88.37% <100.00%> (+1.19%)`	⬆️
generated/definitions/ServicePayload.ts
generated/api-admin/Logo.ts
generated/definitions/DepartmentName.ts
...enerated/definitions/SubscriptionKeyTypePayload.ts
generated/notifications/ProblemJson.ts
generated/definitions/FiscalCode.ts
generated/notifications/requestTypes.ts
... and 10 more

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 fa3bbb6...25e0975. Read the comment docs.

pagopa-github-bot · 2020-10-26T12:16:14Z

Affected stories

🌟 #175395536: Come SAS, voglio poter caricare l' immagine relativa al logo dell' organizzazione utilizzando le API di onboarding programmatico dei servizi.

Generated by 🚫 dangerJS

balanza · 2020-10-26T18:33:11Z

openapi/index.yaml

+    parameters:
+      - name: organization_fiscal_code
+        in: path
+        type: string


Do we have a better definition than string? Can we use OrganizationFiscalCode

balanza · 2020-10-26T18:34:26Z

openapi/index.yaml

+        "200":
+          description: Logo uploaded.


Do we prefer/can return 201 along with the reference to the just uploaded image?

The io-functions-admin endpoint, that is called inside this endpoint on io-functions-services' side, returns 201 with the link to the uploaded image. I don't remember well, but in this case we are avoiding to expose infos about storage and CDN links to external subjects. cc: @gunzip

balanza · 2020-10-26T18:35:16Z

openapi/index.yaml

+        "400":
+          description: Invalid payload.
+          schema:
+            $ref: "#/definitions/ProblemJson"
+        "401":
+          description: Unauthorized.
+        "403":
+          description: Forbidden.
+        "500":
+          description: The service logo cannot be uploaded.
+          schema:
+            $ref: "#/definitions/ProblemJson"


Maybe we want to add 404 in case no Organization is found with such fiscal code?

Nope, there's another story to achieve this. At the moment we don't have a check mechanism cause Organization Model is missing.

The point is that the handler can actually return 404

balanza · 2020-10-26T18:35:52Z

package.json

@@ -17,7 +17,7 @@
    "lint": "tslint -p .",
    "generate:definitions": "rimraf ./generated/definitions && shx mkdir -p ./generated/definitions && gen-api-models --api-spec ./openapi/index.yaml --no-strict --out-dir ./generated/definitions",
    "generate:api-notifications": "rimraf ./generated/notifications && shx mkdir -p ./generated/notifications && gen-api-models --api-spec https://raw.githubusercontent.com/pagopa/io-backend/master/api_notifications.yaml --out-dir ./generated/notifications --response-decoders --request-types",
-    "generate:api-admin": "rimraf generated/api-admin && shx mkdir -p generated/api-admin && gen-api-models --api-spec https://raw.githubusercontent.com/pagopa/io-functions-admin/master/openapi/index.yaml --no-strict --out-dir generated/api-admin --request-types --response-decoders --client",
+    "generate:api-admin": "rimraf generated/api-admin && shx mkdir -p generated/api-admin && gen-api-models --api-spec https://raw.githubusercontent.com/pagopa/io-functions-admin/175395536_upload_organization_logo/openapi/index.yaml --no-strict --out-dir generated/api-admin --request-types --response-decoders --client",


This should not have been committed

Yep, It's a Draft for this reason :)

balanza · 2020-10-26T18:36:55Z

UploadOrganizationLogo/handler.ts

+  | IResponseSuccessJson<undefined>
+  | IResponseErrorUnauthorized
+  | IResponseErrorForbiddenNotAuthorized
+  | IResponseErrorNotFound


We must map it in the spec as well

balanza · 2020-10-26T18:38:51Z

UploadOrganizationLogo/handler.ts

+  apiClient: APIClient,
+  organizationFiscalCode: OrganizationFiscalCode,
+  logo: Logo
+): TaskEither<ErrorResponses, IResponseSuccessJson<undefined>> =>


SInce we are returning undefined, is that a hint that we might return something else (like 202 o0r 204)?

balanza · 2020-10-26T18:40:54Z

UploadOrganizationLogo/__tests__/handler.test.ts

+};
+
+describe("UploadOrganizationLogo", () => {
+  it("should respond with 201 if logo upload was successfull", async () => {


Why 201? I think is 200, according to the implementation

Yes, this must be corrected 👍

balanza · 2020-10-26T18:42:17Z

UploadOrganizationLogo/__tests__/handler.test.ts

+      aLogoPayload
+    );
+
+    expect(apiClientMock.uploadOrganizationLogo).toHaveBeenCalledTimes(1);


Do we need this test? This function is mocked, so we expect the test to just fail if it's not called

balanza · 2020-10-26T18:42:26Z

UploadOrganizationLogo/__tests__/handler.test.ts

+      aLogoPayload
+    );
+
+    expect(apiClientMock.uploadOrganizationLogo).toHaveBeenCalledTimes(1);


balanza · 2020-10-26T18:42:33Z

UploadOrganizationLogo/__tests__/handler.test.ts

+      aLogoPayload
+    );
+
+    expect(apiClientMock.uploadOrganizationLogo).toHaveBeenCalledTimes(1);


balanza

I made a lot of comments and they might appear unrelated and conflicting. Here I try to sum up:

Implementation and specification are not aligned
We might want to model the success case with something different than 200
Can we use OrganizationFiscaleCode definition?
We should avoid testing implementation (that is, has this been called) and try to test input/output instead

AleDore · 2020-10-28T16:46:37Z

Can we merge this? @gunzip @balanza

gunzip · 2020-10-28T20:54:58Z

UploadOrganizationLogo/handler.ts

+  ).map(_ => ResponseSuccessAccepted());
+
+/**
+ * Handles requests for upload a service logo by a service ID and a base64 logo' s string.


Suggested change

* Handles requests for upload a service logo by a service ID and a base64 logo' s string.

* Handles requests for upload an organization logo.

gunzip · 2020-10-28T20:56:44Z

openapi/index.yaml

+        This API operation needs a valid API key
+        otherwise 403 forbidden will be returned to the caller.


this is implied by the 403 status below

gunzip · 2020-10-28T20:57:35Z

openapi/index.yaml

+  OrganizationFiscalCode:
+    name: organization_fiscal_code
+    in: path
+    type: string
+    description: Organization fiscal code.
+    format: OrganizationFiscalCode
+    minLength: 11
+    maxLength: 11
+    pattern: "^[0-9]{11}$"
+    required: true
+    x-example: "12345678901"


you should use this format:

https://github.com/pagopa/io-functions-commons/blob/master/openapi/definitions.yaml#L290

[#175395536] Add upload organization logo

9a204b0

AleDore requested review from gunzip and francescopersico October 26, 2020 12:14

AleDore requested a review from balanza October 26, 2020 15:12

AleDore changed the title ~~[#175395536] Add upload organization logo~~ [#175395536] Add Organization logo's upload Oct 26, 2020

[#175395536] Removed useless tests

25e0975

balanza suggested changes Oct 26, 2020

View reviewed changes

AleDore added 2 commits October 27, 2020 14:02

[#175395536] Refactor over review

b27cb06

[#175395536] Rebase

fc08559

AleDore marked this pull request as ready for review October 27, 2020 13:04

AleDore requested a review from BurnedMarshal as a code owner October 27, 2020 13:04

AleDore requested a review from balanza October 27, 2020 13:05

[#175395536] Rebase

40b2530

gunzip reviewed Oct 28, 2020

View reviewed changes

gunzip requested changes Oct 28, 2020

View reviewed changes

[#175395536] Refactor over review

090232f

AleDore requested a review from gunzip October 29, 2020 08:53

gunzip approved these changes Oct 29, 2020

View reviewed changes

gunzip merged commit 325fa95 into master Oct 29, 2020

gunzip deleted the 175395536_add_upload_organization_logo branch October 29, 2020 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#175395536] Add Organization logo's upload #83

[#175395536] Add Organization logo's upload #83

AleDore commented Oct 26, 2020

codecov-io commented Oct 26, 2020 •

edited

Loading

pagopa-github-bot commented Oct 26, 2020 •

edited

Loading

balanza Oct 26, 2020

balanza Oct 26, 2020

AleDore Oct 27, 2020

balanza Oct 26, 2020

AleDore Oct 27, 2020

balanza Oct 27, 2020

balanza Oct 26, 2020

AleDore Oct 27, 2020

balanza Oct 26, 2020

balanza Oct 26, 2020

balanza Oct 26, 2020

AleDore Oct 27, 2020

balanza Oct 26, 2020

balanza Oct 26, 2020

balanza Oct 26, 2020

balanza left a comment •

edited

Loading

AleDore commented Oct 28, 2020

gunzip Oct 28, 2020

gunzip Oct 28, 2020

gunzip Oct 28, 2020

	* Handles requests for upload a service logo by a service ID and a base64 logo' s string.
	* Handles requests for upload an organization logo.

		This API operation needs a valid API key
		otherwise 403 forbidden will be returned to the caller.

[#175395536] Add Organization logo's upload #83

[#175395536] Add Organization logo's upload #83

Conversation

AleDore commented Oct 26, 2020

codecov-io commented Oct 26, 2020 • edited Loading

Codecov Report

pagopa-github-bot commented Oct 26, 2020 • edited Loading

Affected stories

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balanza left a comment • edited Loading

Choose a reason for hiding this comment

AleDore commented Oct 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 26, 2020 •

edited

Loading

pagopa-github-bot commented Oct 26, 2020 •

edited

Loading

balanza left a comment •

edited

Loading