twentyhq / twenty

Building a modern alternative to Salesforce, powered by the community.
https://twenty.com
GNU Affero General Public License v3.0
16.07k stars 1.82k forks source link

Ex: `additionalEmails` spec'd as object(key value pair) in PATCH, POST in the API documentation #7262

Open LucasZapico opened 1 week ago

LucasZapico commented 1 week ago

Bug Description

additionalEmails property is defined as an object in the PATCH and POST definitions in the API documentation when an array is required for it to render in the GUI. Additionally, if an object additonalEmails is sent as POST and PATCH the api response 201 created but this is not reflected in the app(gui).

Technical Information

I believe the API documentation is custom-generated from each Twenty instance so the links below is not is just a compass.

POST - Twenty API doc

  "emails": {
    "primaryEmail": "string",
    "additionalEmails": {} // <-- defined as object but requires [] to render in GUI 
  },

Twenty API docs

   "emails": {
        "primaryEmail": "string",
        "additionalEmails": {} <--- defined as object
      },

Further unexpected behavior here because the GET{id} returns the the object additionalEmails

//....
"emails": {
  "primaryEmail": "bar99@example.com",
  "additionalEmails": {
    "1": "bar100@example.com",
    "2": "bar101@example.com"
   }
},
//....

Example

POST additionalEmails as object

{
  "tags": "TESTING",
  "emails": {
    "primaryEmail": "bar99@example.com",
    "additionalEmails": {"1": "bar100@example.com", "2": "bar101@example.com"}
  },

  "name": {
    "firstName": "bar99",
    "lastName": "bar99last"
  },
    "phone" : "1112223344"

}

Response

//......
"emails": {
    "primaryEmail": "bar99@example.com",
    "additionalEmails": {
        "1": "bar100@example.com",
        "2": "bar101@example.com"
    }
},
//.....

But no email is rendered in the app record.

POST additionalEmails as array

{
  "tags": "TESTING",
  "emails": {
    "primaryEmail": "zob99@example.com",
    "additionalEmails": [ "1", "zob100@example.com", "zob@example.com"]
  },

  "name": {
    "firstName": "zobb1",
    "lastName": "zab1last"
  },
    "phone" : "1112223344"

}

response 201

//... stuff
"emails": {
    "primaryEmail": "zob99@example.com",
    "additionalEmails": [
        "1",
        "zob100@example.com",
        "zob@example.com"
    ]
},
//... stuff

I added "1" as an email here just for kick and it returns valid and is rendered in the email array in the GUI

Conclusion

Seems to be some missing validations here and needed clarity as to how Twenty wants to handle associated emails to a record.

Thoughts

There are a few ways to take this information. If Twenty wants to handle both, arrays and key-value pairs for additonalEmails the GUI needs to be able to handle both formats and the API docs need to reflect. Ideally the app would have some validation of types and through appropriate errors to the request.

Express-validator handles email validation and might have some patterns that are worth a look.

Additionally, email string validation as a togglable feature for the email type is what I expect. I have used regex checks in my projects in the passed but I know a lot of people don't like regex checks.

Reasonable Answer

([-!#-'*+/-9=?A-Z^-~]+(\.[-!#-'*+/-9=?A-Z^-~]+)*|"([]!#-[^-~ \t]|(\\[\t -~]))+")@[0-9A-Za-z]([0-9A-Za-z-]{0,61}[0-9A-Za-z])?(\.[0-9A-Za-z]([0-9A-Za-z-]{0,61}[0-9A-Za-z])?)+

But there are a lot of opinions on this discussed here 👇

[validate email address regex - stackoverflow](https://stackoverflow.com/questions/201323/how-can-i-validate-an-email-address-using-a-regular-expression

Final

I hope this issue was conveyed correctly and my opinions and thoughts do not come through in an overly critical tone. I appreciate this project immensely.

Best

Bonapara commented 1 week ago

@ijreilly What do you think?

FelixMalfait commented 1 week ago

Good point - we should be consistent and return an array not an object. I believe that's already the case in our GraphQL API.

@martmull if you don't mind having a look when you're back, thanks!

As for validation, I agree it'd be great but I think we'd want to introduce a more global framework for backend-side validation for every field type, this is not planned for the near term, probably early next year