[Refactoring] [Arch] Upload Profile Picture by Sobowale-Olayiwola · Pull Request #77 · CollActionteam/collaction_backend

Sobowale-Olayiwola · 2022-02-10T00:39:52Z

No description provided.

rubenhorn

For the sake of completeness, please add a test that checks if the service.GetUploadUrl calls repository.GetUploadUrl with the same parameters. (see https://github.com/CollActionteam/collaction_backend/blob/development/internal/contact/email_test.go#L35)

rubenhorn · 2022-02-13T11:42:16Z

+
+	// This is to recover from a "invalid memory address or nil pointer dereference: errorString" runtime error
+	// when the endpoint is called without valid auth token
+	defer func() {


Is this function still needed, since below the error is checked and "user not authorized" may be returned?

rubenhorn · 2022-02-13T11:55:05Z

+		return res, err
+	}
+
+	// getting user id, which will be used as object key


Redundant comment. Please remove.

Noted, I left the comment just as it was in the code before but I would remove them

I generally refactored the code the into the hexagonal structure without changing most of the logic since it was working like that before. The recover function isn't needed since err is being checked and sent to the user

I would go over it and remove unecessary code

rubenhorn · 2022-02-13T11:59:23Z

+		filekey = userID + "." + ext
+	)
+
+	// Initialize a session that the SDK will use to load


I don't think these comments are helpful. Please remove them.

Sobowale-Olayiwola · 2022-02-13T12:58:36Z

For the sake of completeness, please add a test that checks if the service.GetUploadUrl calls repository.GetUploadUrl with the same parameters. (see https://github.com/CollActionteam/collaction_backend/blob/development/internal/contact/email_test.go#L35)

I would add test for the sake of completeness

mrsoftware · 2022-02-13T14:17:58Z

+	return &image{imageUploadRepository: profileImageUploadRepo}
+}
+
+func (i *image) GetUploadUrl(ext string, userID string) (string, error) {


always get context as the first arg

mrsoftware · 2022-02-13T14:20:31Z

+	"github.com/aws/aws-sdk-go/aws/session"
+)
+
+func handler(ctx context.Context, req events.APIGatewayV2HTTPRequest) (res events.APIGatewayProxyResponse, err error) {


use events.APIGatewayV2HTTPResponse

Can you kindly explain why?

we are using events.APIGatewayV2HTTPRequest for req and the equivalent of this in response type is events.APIGatewayV2HTTPResponse.
no critical requirement to change it.

mrsoftware · 2022-02-13T14:29:08Z

+	// This is to recover from a "invalid memory address or nil pointer dereference: errorString" runtime error
+	// when the endpoint is called without valid auth token
+	defer func() {
+		if r := recover(); r != nil {


can you find the source of panic and do some checks to prevent panic?

I think it is basically the error for checking if the user is authenticated, so there is no need to panic

as we don't know if is there any possibility of panic, it's best to keep this code, but log the panic reason to fix it in the future

The function can't be called without a valid user, because the API Gateway enforces this through the Authorizer that is used on this handler (see /template.yaml).

rubenhorn · 2022-02-13T14:30:30Z

NOTE: I think the checks fail because you are working no a fork of the repo (as opposed to a branch in the original repo). Thus, the secrets containing the AWS credentials are missing.

(Not need to take any action)

Sobowale-Olayiwola · 2022-02-13T18:49:30Z

NOTE: I think the checks fail because you are working no a fork of the repo (as opposed to a branch in the original repo). Thus, the secrets containing the AWS credentials are missing.

(Not need to take any action)

I am working on a forked copy on my end, is there need to create a new branch and work directly on the repo?

rubenhorn · 2022-02-13T18:57:50Z

NOTE: I think the checks fail because you are working no a fork of the repo (as opposed to a branch in the original repo). Thus, the secrets containing the AWS credentials are missing.
(Not need to take any action)

I am working on a forked copy on my end, is there need to create a new branch and work directly on the repo?

You don't need to do it for this PR, but I would recommend that you work in the original repo for any future features.
It is just more convenient, because a reviewer can directly see if the tests passed and doesn't have to run them himself.

rubenhorn · 2022-02-13T19:00:18Z

Please delete the old code that is being replaced and update /template.yaml to run the new code.

Sobowale-Olayiwola · 2022-02-13T19:31:36Z

NOTE: I think the checks fail because you are working no a fork of the repo (as opposed to a branch in the original repo). Thus, the secrets containing the AWS credentials are missing.
(Not need to take any action)

I am working on a forked copy on my end, is there need to create a new branch and work directly on the repo?

You don't need to do it for this PR, but I would recommend that you work in the original repo for any future features. It is just more convenient, because a reviewer can directly see if the tests passed and doesn't have to run them himself.

Okay noted

Sobowale-Olayiwola · 2022-02-13T19:32:04Z

Please delete the old code that is being replaced and update /template.yaml to run the new code.

Okay noted would work on that

mrsoftware · 2022-02-22T09:47:35Z

+	"github.com/aws/aws-sdk-go/aws/session"
+)
+
+func handler(ctx context.Context, req events.APIGatewayV2HTTPRequest) (res events.APIGatewayProxyResponse, err error) {


we are using events.APIGatewayV2HTTPRequest for req and the equivalent of this in response type is events.APIGatewayV2HTTPResponse.
no critical requirement to change it.

mrsoftware · 2022-02-22T09:50:10Z

+	// This is to recover from a "invalid memory address or nil pointer dereference: errorString" runtime error
+	// when the endpoint is called without valid auth token
+	defer func() {
+		if r := recover(); r != nil {


as we don't know if is there any possibility of panic, it's best to keep this code, but log the panic reason to fix it in the future

rubenhorn · 2022-03-11T17:56:11Z

closes #68

rubenhorn

Looks good.
Tho I still think pkg/handler/aws/uploadProfilePicture/main.go:22 can be removed.

Sobowale-Olayiwola · 2022-03-13T18:06:01Z

The defer func(){}, yeah some reviews was like I should leave it.So should I remove it?

rubenhorn · 2022-03-13T18:09:46Z

The defer func(){}, yeah some reviews was like I should leave it.So should I remove it?

I think so.
We are now aware of this and can add it back in if we should indeed need it (unlikely)

Sobowale-Olayiwola · 2022-03-13T18:17:14Z

I have updated it

…

On Sun, 13 Mar 2022 at 19:09, Ruben Horn ***@***.***> wrote: The defer func(){}, yeah some reviews was like I should leave it.So should I remove it? I think so. We are now aware of this and can add it back in if we should indeed need it (unlikely) — Reply to this email directly, view it on GitHub <#77 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARCSDIJAMYF7NWHVV66KAO3U7YVPJANCNFSM5N7C2UDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Co-authored-by: Mohammad Rajabloo <themrsoftware@gmail.com>

…action_backend into feat/hex-arch/68

commit 29442c1 Merge: 9a473b1 fd30c56 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sat Jun 11 19:03:51 2022 +0200 Merge branch 'development' into feat/gh-73/crowdactions commit 9a473b1 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sat Jun 11 19:00:07 2022 +0200 returning crowdaction object commit fd30c56 Merge: d189c2d 15a4d6e Author: Mathias Mogensen <42929161+Xazin@users.noreply.github.com> Date: Wed May 18 09:40:16 2022 +0200 Merge pull request #132 from CollActionteam/feat/gh-73/crowdactions Feat/gh 73/crowdactions commit 15a4d6e Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Tue May 17 23:26:57 2022 +0200 comment out cms endpoint (revisit later) commit 9f8fb11 Merge: 141e144 d189c2d Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sun May 15 09:28:12 2022 +0200 Merge branch 'development' of github.com:CollActionteam/collaction_backend into feat/gh-73/crowdactions commit 141e144 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sun May 15 09:24:31 2022 +0200 ⚡️ scan operations on crowdactions commit 2dd48b2 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sun May 15 09:23:20 2022 +0200 ⚡️ mock create crowdaction commit d189c2d Merge: 3fb6e82 dce91c7 Author: Mathias Mogensen <42929161+Xazin@users.noreply.github.com> Date: Thu May 5 21:50:41 2022 +0200 Merge pull request #129 from CollActionteam/fix/contact-form-unit-tests Expect recipient also as sender in tests commit dce91c7 Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Thu May 5 21:43:56 2022 +0200 Expect recipient also as sender in tests commit 3fb6e82 Merge: 1366e1e 7a51f51 Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Thu May 5 21:31:00 2022 +0200 Merge pull request #128 from CollActionteam/fix/email-contact-not-registered Use registered address as both sender and recipient commit 7a51f51 Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Thu May 5 21:29:15 2022 +0200 Use registered address as both sender and recipient commit 1366e1e Merge: 93bb124 8b63733 Author: Mathias Mogensen <42929161+Xazin@users.noreply.github.com> Date: Thu May 5 20:50:42 2022 +0200 Merge pull request #126 from CollActionteam/fix/app-version-regex Fixed regex commit 8b63733 Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Thu May 5 20:49:57 2022 +0200 Fixed regex s. https://regexr.com/6l215 commit 93bb124 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sun Apr 17 23:15:50 2022 +0200 Feat/gh 73/crowdactions (#119) * 🚀 crowdaction hex refactoring init * 🚧 dynamodb manager repository, pt1: init * ⚡️ launching own sam config * 🚧 [crowdaction] deployed first function on to aws * 🚧 [crowdaction] removing extra copy of dynamo repository * 🚧 [crowdaction] init model and validation on request * 🚧 [crowdaction] changed the model function names * 🚧 [crowdaction] working on internal and handler * 🚧 [crowdaction] simple version of GET request by ID * 🚧 [crowdaction] testing GET request by ID * 🚧 [crowdaction] creating external function to GET crowdaction by ID * 🚧 [crowdaction] including query function in ddb repository * 🚧 [crowdaction] initial code base for GET crowdactions by status * 🚧 [crowdaction] cleaning up, commitment options * 🚧 [crowdaction] including the mock repository for dynamo and crowdaction * 🚧 [crowdaction] modifying the mock repository to include GetCrowdactionByStatus * 🚧 [crowdaction] removed unnecessary comments, per the feedback on pull request * [pull request] removing samconfig file from history * [pull request] ignoring samconfig file as per the pull request * [pull request] modified naming for the internal interface * [pull request] getting rid of useless else statement * [pull request] removing dynamodb dependencies in logic * [pull request] including mock repository and crowdaction test * [pull request] cleaning up some unnecessary lines * [pull request] separating repository concerns * [pull request] modifying test cases with new repository structure * [pull request] changing dynamo repository call from crowdaction main handler * [pull request] changing parameters to pass unit test case * [pull request] changing crowdactionID from test case * [pull request] changing method call for crowdaction test * [pull request] fixing parameter issue in crowdaction_test * [pull request] testing issue with the args.get(x) call in mock/dynamo repository * [pull request] including Mock in each of the function calls from the dynamo repository * [pull request] modifying test cases for crowdaction * [pull request] changes done based on feedback [pull request] ignoring samconfig file as per the pull request [pull request] modified naming for the internal interface [pull request] getting rid of useless else statement [pull request] removing dynamodb dependencies in logic [pull request] including mock repository and crowdaction test [pull request] cleaning up some unnecessary lines [pull request] separating repository concerns [pull request] modifying test cases with new repository structure [pull request] changing dynamo repository call from crowdaction main handler [pull request] changing parameters to pass unit test case [pull request] changing crowdactionID from test case [pull request] changing method call for crowdaction test [pull request] fixing parameter issue in crowdaction_test [pull request] testing issue with the args.get(x) call in mock/dynamo repository [pull request] including Mock in each of the function calls from the dynamo repository [pull request] modifying test cases for crowdaction * [pull request] removing unnecessary hello world * [pull request] removing some unnecessary comments * final changes before merging This commit starts by containing the separation between the post and the get methods that will be used for handling crowdactions * removing a duplicate package When merged, the net/http packe was imported twice by accident * [pull request] cleaning up some final comments This should be the last commit before merging to development. The implementation includes the initial basis for the /post endpoint to create crowdactions from the CMS. The continuation of this task (/post) should be done ideally in the CMS task (up for a discussion, of course) but could expanded somewhere else for other purposes. For example, the badge issue may implement a very rudimentary version of this /post crowdaction for testing purposes. * changing the crowdaction test case commit 854d2bc Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sun Apr 17 14:28:41 2022 +0200 changing the crowdaction test case commit 188f7b3 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Sun Apr 17 11:35:33 2022 +0200 [pull request] cleaning up some final comments This should be the last commit before merging to development. The implementation includes the initial basis for the /post endpoint to create crowdactions from the CMS. The continuation of this task (/post) should be done ideally in the CMS task (up for a discussion, of course) but could expanded somewhere else for other purposes. For example, the badge issue may implement a very rudimentary version of this /post crowdaction for testing purposes. commit e7a3d2a Merge: eb3b973 9ac0b81 Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Sat Apr 16 19:57:25 2022 +0200 Merge pull request #123 from CollActionteam/Automated-security-scanning Automated-security-scanning commit f1e7306 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Fri Apr 15 21:25:11 2022 +0200 removing a duplicate package When merged, the net/http packe was imported twice by accident commit f8e0936 Merge: b9dd8b7 eb3b973 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Fri Apr 15 21:23:54 2022 +0200 Merge branch 'development' of github.com:CollActionteam/collaction_backend into feat/gh-73/crowdactions # Conflicts: # pkg/handler/aws/emailContact/main.go commit b9dd8b7 Author: Ed Reinoso <edgardojesus16@gmail.com> Date: Fri Apr 15 21:18:53 2022 +0200 final changes before merging This commit starts by containing the separation between the post and the get methods that will be used for handling crowdactions commit 9ac0b81 Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Fri Apr 15 12:04:21 2022 +0200 Create codeql-analysis.yml commit eb3b973 Merge: 8626468 824b5ed Author: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Date: Mon Apr 11 01:07:09 2022 +0200 Merge pull request #122 from CollActionteam/bugfix/contact_valication fix app version validation commit 8626468 Author: Sobowale-Olayiwola <71639457+Sobowale-Olayiwola@users.noreply.github.com> Date: Sun Apr 10 22:57:37 2022 +0100 [Refactoring] [Arch] Upload Profile Picture (#77) * Added interfaces for profile images upload * Implmented repository functions * created handler for profile pics upload * Updated code based on reviews * Removed comments based on reviews * Added mock and test * Updated template.yaml * Removed deferred recovery function call and updated error msg * Update pkg/repository/aws/uploadProfilePictureManager.go Co-authored-by: Mohammad Rajabloo <themrsoftware@gmail.com> * Fixed changes regarding review Co-authored-by: Ruben Horn <5367484+rubenhorn@users.noreply.github.com> Co-authored-by: Mohammad Rajabloo <themrsoftware@gmail.com> commit 824b5ed Author: Mohammad Rajabloo <themrsoftware@gmail.com> Date: Mon Apr 11 00:42:43 2022 +0430 fix app version validation

Sobowale-Olayiwola added 3 commits February 10, 2022 01:34

Added interfaces for profile images upload

c1584f3

Implmented repository functions

40823fd

created handler for profile pics upload

4e7d81e

Sobowale-Olayiwola requested review from FaheemuddinKhan, micheldias, midepeter, mrsoftware, nielsGal and rubenhorn February 10, 2022 00:41

rubenhorn suggested changes Feb 13, 2022

View reviewed changes

mrsoftware reviewed Feb 13, 2022

View reviewed changes

rubenhorn changed the title ~~Feat/hex-arch/68~~ [Refactoring] [Arch] Upload Profile Picture Feb 13, 2022

mrsoftware suggested changes Feb 22, 2022

View reviewed changes

Sobowale-Olayiwola added 4 commits March 13, 2022 14:16

Updated code based on reviews

e7c790f

Removed comments based on reviews

0fb8505

Added mock and test

39cfa35

Updated template.yaml

0f21a2b

rubenhorn previously approved these changes Mar 13, 2022

View reviewed changes

Removed deferred recovery function call and updated error msg

d8e793f

Sobowale-Olayiwola dismissed rubenhorn’s stale review via d8e793f March 13, 2022 18:16

rubenhorn previously approved these changes Mar 13, 2022

View reviewed changes

NwokoyeChigozie suggested changes Mar 18, 2022

View reviewed changes

Comment thread pkg/handler/aws/uploadProfilePicture/main.go

Comment thread pkg/handler/aws/uploadProfilePicture/main.go

Sobowale-Olayiwola requested review from NwokoyeChigozie and mrsoftware March 18, 2022 08:18

NwokoyeChigozie previously approved these changes Mar 18, 2022

View reviewed changes

Update pkg/repository/aws/uploadProfilePictureManager.go

3bcfd9f

Co-authored-by: Mohammad Rajabloo <themrsoftware@gmail.com>

rubenhorn dismissed stale reviews from NwokoyeChigozie and themself via 3bcfd9f March 19, 2022 19:11

mrsoftware reviewed Mar 23, 2022

View reviewed changes

Comment thread pkg/mocks/repository/uploadProfilePictureManager.go

rubenhorn previously approved these changes Apr 2, 2022

View reviewed changes

Sobowale-Olayiwola added 2 commits April 4, 2022 08:02

Fixed changes regarding review

55bd285

Merge branch 'feat/hex-arch/68' of github.com:Sobowale-Olayiwola/coll…

4cfb1e0

…action_backend into feat/hex-arch/68

Sobowale-Olayiwola dismissed rubenhorn’s stale review via 4cfb1e0 April 4, 2022 07:02

mrsoftware approved these changes Apr 10, 2022

View reviewed changes

rubenhorn merged commit 8626468 into CollActionteam:development Apr 10, 2022

Uh oh!

Conversation

Sobowale-Olayiwola commented Feb 10, 2022

Uh oh!

rubenhorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sobowale-Olayiwola commented Feb 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenhorn commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sobowale-Olayiwola commented Feb 13, 2022

Uh oh!

rubenhorn commented Feb 13, 2022

Uh oh!

rubenhorn commented Feb 13, 2022

Uh oh!

Sobowale-Olayiwola commented Feb 13, 2022

Uh oh!

Sobowale-Olayiwola commented Feb 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenhorn commented Mar 11, 2022

Uh oh!

rubenhorn left a comment

Choose a reason for hiding this comment

Uh oh!

Sobowale-Olayiwola commented Mar 13, 2022

Uh oh!

rubenhorn commented Mar 13, 2022

Uh oh!

Sobowale-Olayiwola commented Mar 13, 2022 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rubenhorn commented Feb 13, 2022 •

edited

Loading