As an API user, I shouldn't be able to update isCustom on an object or a field metadata

Weiko commented 3 months ago

Bug Description

Many part of the code assume that isCustom:true is from the user input and should be ignore whereas isCustom:false is standard and should be kept in sync with the codebase entities. However, even if an object or a field is automatically created as isCustom: true via the API, we can also update those field/objects and change that value which will break a few parts of the app including metadata sync scripts.

mutation UpdateOneField($input: UpdateOneFieldMetadataInput!) {
  updateOneField(input: $input) {
    id
    isCustom
  }
}

This input should not be possible, isCustom should not be available.

{
  "input": {
    "id": "1a3df789-8f19-4962-be35-34f59001b966"
  "update": {
    "isCustom": false
  }
}

Technical inputs

Do not expose isCustom in the graphql input field type

Faisal-imtiyaz123 commented 3 months ago

@Weiko I would like to work on it.

prateekj117 commented 3 months ago

@Weiko @charlesBochet @FelixMalfait Can I take this up? Seems quite straightforward. I see we need to remove the isCustom from the twenty-server from class UpdateFieldInput. And also remove it from UpdateFieldInput defined on frontend. I don't see any tests breaking by this change from code, but if would be there, will fix it too.

FelixMalfait commented 3 months ago

@prateekj117 thanks for providing a plan! I think that could work. We want to do it both at the field level and the object level. We need to make sure custom objects are always created with isCustom=true then, and that standard objects are always created with isCustom=false, even if it's not set by the api, which means it needs to be set at a different level in code. Currently the default value for isCustom is false, it might have been slightly easier if the value had been true, but not sure. TBC if a change is needed. Assigning you if you're still up for it?

@Faisal-imtiyaz123 thanks again! don't hesitate to give directions when you're looking to take something, that way it's helpful to validate it together.

Mohamedkaif10 commented 3 months ago

@Weiko @FelixMalfait I would like to work on this issue.

FelixMalfait commented 3 months ago

Maybe @Faisal-imtiyaz123 wants to grab it first? Since @prateekj117 didn't reply. Otherwise @Mohamedkaif10 go ahead in case @Faisal-imtiyaz123 doesn't reply in 24hrs. Thanks to all of you!

Faisal-imtiyaz123 commented 3 months ago

@FelixMalfait You may assign it to @Mohamedkaif10 .

FelixMalfait commented 3 months ago

Done! @charlesBochet marked this as a good first issue and it's doable, but I wouldn't say it's very easy as it requires at least a good understanding of what standard VS custom objects are and how they behave differently

Mohamedkaif10 commented 3 months ago

Yeah now , i removed isCustom from the twenty-frontend(graphql.ts file) and also from mutation. suggest if other changes needed ..@FelixMalfait

Bonapara commented 2 months ago

/oss.gg 150

oss-gg[bot] commented 2 months ago

Thanks for opening an issue! It's live on oss.gg!