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

Add minio storage #121

Open
wants to merge 2 commits into
base: v2
Choose a base branch
from
Open

Add minio storage #121

wants to merge 2 commits into from

Conversation

nguyenk
Copy link
Member

@nguyenk nguyenk commented Oct 23, 2023

No description provided.

@nguyenk nguyenk requested review from Ngob and homersimpsons October 23, 2023 05:17
@nguyenk nguyenk force-pushed the feat-v2/add-minio-storage branch from 145aed3 to d7683ef Compare October 23, 2023 05:25

if ($this->createBucket($this->publicBucketName, $style)) {
//Add DL rights for public bucket
$policyReadOnly = '{
Copy link

Choose a reason for hiding this comment

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

injecter la policy depuis le services.yml (en tant que parameter)


use Symfony\Component\HttpFoundation\File\UploadedFile;

trait ProfilePicture
Copy link

@Ngob Ngob Oct 23, 2023

Choose a reason for hiding this comment

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

can we use a validation constraint on file? I think https://symfony.com/doc/current/reference/constraints/Image.html should be good. We may need to add a max file size value aswell

Additionnaly, We should suffix trait with Trait (we should probably prefer class composition over traits)


use Symfony\Component\Validator\Constraints as Assert;

class CreateUserDto
{
use ProfilePicture;
Copy link

Choose a reason for hiding this comment

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

je comprend pas l'intérêt du trait alors que c'est l'unique endroit où il est utilisé. Peut être créer un trait plus générique (du style, ImageTrait) mais je ne suis pas sûr de l'utilité

return $this->name;
}

public function setName(string $name): static
Copy link

Choose a reason for hiding this comment

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

I personally dont like "fluent interface" or "return this" in most cases, I think its less readable in most cases (more than one execution per line + you can't easily know what is going on, for example $user->setName("foo")->calculateTime()->getCompany()->setName(), or worst $company->setUser($user->setName("foo"))). (weirdly the doctrine creator seems to agree with me )
We should decide on a general guideline about that


export default async function useGetCompany(companyId: string) {
return useAppFetch<Company>(() => "/companies/" + companyId, {
key: "getCompany",
Copy link

Choose a reason for hiding this comment

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

remove the key usage or use the companyId in the key

import { GET } from "~/constants/http";
import useAppFetch from "~/composables/useAppFetch";

export default async function useListUsers() {
Copy link

Choose a reason for hiding this comment

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

this is suspicious (wrong name)


const route = useRoute();

const { data: company, pending: companyPending } = await useGetCompany(
Copy link

Choose a reason for hiding this comment

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

you should use the {{ error }}

@click="onDeleteCompany(company)"
>
Delete
</button>
Copy link

Choose a reason for hiding this comment

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

use prime components?


const onDeleteCompany = async (company: Company) => {
try {
await deleteCompany(company);
Copy link

Choose a reason for hiding this comment

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

error should be handled

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.

2 participants