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

feat: prepare AWS S3 upload for avatars #1747

Merged
merged 14 commits into from
Sep 2, 2018
Merged

feat: prepare AWS S3 upload for avatars #1747

merged 14 commits into from
Sep 2, 2018

Conversation

asbiin
Copy link
Member

@asbiin asbiin commented Aug 27, 2018

Fix the feature of upload files to AWS S3 for avatars.
This is linked to #1742

Copy link

@sonarcloud sonarcloud bot left a comment

Choose a reason for hiding this comment

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

SonarQube analysis found issues:
Bug Bugs: 0
Vulnerability Vulnerabilities: 0
Code Smell Code Smells: 2

Including the following issue(s) which could not be reported in line:

  1. Code Smell Code Smell: Refactor this function to reduce its Cognitive Complexity from 16 to the 15 allowed. (more)

See all issues in SonarCloud

use Illuminate\Console\Command;
use Illuminate\Console\ConfirmableTrait;

class MoveAvatars extends Command
Copy link
Member

Choose a reason for hiding this comment

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

This is something that should be done only once, right? Maybe we could indicate it in the description.

Copy link
Member Author

Choose a reason for hiding this comment

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

As you know, the location of an avatar is stored in Contact -> avatar_location property. The MoveAvatars command aims to move avatars (contacts with has_avatar to true) from current location to the - possibly different - default driver, as config('filesystems.default'). This way, you can run the commands multiple times: if the current location of the avatar is the default one, it's not touched.

Copy link
Member

Choose a reason for hiding this comment

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

But will we have to run the commands multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

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

We will have to run the command one time for this migration, but it could be run multiple times, every time you update the default filesystem store.

$filename = pathinfo($contact->avatar_file_name, PATHINFO_FILENAME);
$extension = pathinfo($contact->avatar_file_name, PATHINFO_EXTENSION);

$avatar_file_name = 'avatars/'.$filename.'.'.$extension;
Copy link
Member

Choose a reason for hiding this comment

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

$avatar_file_name -> $avatarFileName for PRS4 compliance.

@@ -19,6 +19,7 @@ class Kernel extends ConsoleKernel
'App\Console\Commands\ImportCSV',
'App\Console\Commands\ImportVCards',
'App\Console\Commands\LangGenerate',
'App\Console\Commands\MoveAvatars',
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could put the commands that should only be done once if a proper folder, like Commands\OneTime or something.

@@ -937,14 +938,50 @@ public function getAvatarURL($size = 110)
return $this->avatar_external_url;
}

$original_avatar_url = Storage::disk($this->avatar_location)->url($this->avatar_file_name);
$original_avatar_url = $this->avatar_file_name;
Copy link
Member

Choose a reason for hiding this comment

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

PSR4

Copy link
Member

@djaiss djaiss left a comment

Choose a reason for hiding this comment

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

A few comments

/**
* Delete avatars files.
*/
public function deleteAvatars()
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need to create a function in Contact object just for this, especially if this process will only happen once for this migration. Is it really useful? Can we move this function elsewhere ?

Copy link
Member Author

Choose a reason for hiding this comment

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

When a contact already has an avatar (not gravatar), if you update the image, today the old image stays in the store forever. I'm not sure it's a voluntary behavior. I've created this function to remove the old image, but we can imagine an other behavior...

Copy link
Member Author

Choose a reason for hiding this comment

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

And today a user can flood us by uploading as much as images he can, and they will stay in the store...

Copy link
Member

Choose a reason for hiding this comment

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

This is true. Thanks for the explanation.
I wonder if we should create a service for this method instead, like what I've been started to do here: https://github.com/monicahq/monica/pull/1629/files#diff-b3c40f0913df027e06af970085f889ed

The idea: have a class called, for instance, deleteAvatarForContact. This is explained here.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

After a bit reflexion: this does not apply here, even I think Services are great, but it's used for Controller, to handle request the same way for htpp and api. In this case, I already have the $contact object, and I just pass it to the Service: there is no interest.

@djaiss
Copy link
Member

djaiss commented Sep 2, 2018

👍

@asbiin asbiin merged commit feded44 into master Sep 2, 2018
@asbiin asbiin deleted the feat/prepare-s3 branch September 2, 2018 14:44
@github-actions
Copy link

github-actions bot commented Feb 2, 2021

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants