Skip to content

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)


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.

3 participants