payload icon indicating copy to clipboard operation
payload copied to clipboard

Feature for supporting hasMany property in text field

Open Gokulsck opened this issue 2 years ago • 8 comments

Description

  • Implemented a feature for handling hasMany property for text fields to store array of string.
  • https://github.com/payloadcms/payload/discussions/4393
  • [x] I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Change to the examples directory (does not affect core functionality)
  • [x] This change requires a documentation update

Checklist:

  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] Existing test suite passes locally with my changes
  • [ ] I have made corresponding changes to the documentation

Gokulsck avatar Dec 25 '23 05:12 Gokulsck

I already worked on the same issue and I also reviewed your changes. I noticed that in the payload-types.ts the type for the hasMany, validateHasMany and so on is still string | null in my case it was set to string [] | null as I also included some changes in the "buildObjectType", "configToJSONSchema", and "buildMutationInputType". Maybe you want to have a look at my forked repo or give me access to work on the PR so I can also include those changes and the unit test I wrote @Gokulsck

Cuzart avatar Dec 27 '23 17:12 Cuzart

@Cuzart I gave you the access could you please check?

Gokulsck avatar Dec 28 '23 04:12 Gokulsck

I handled the cases for the schemas similar to the ones found for number field with has many and fixed the type generation. I changed some code in validations.ts which was causing an unit test to fail and also added one unit test and an e2e test for hasMany on text fields.

The change for postgres seems pretty straightforward but you might consider renaming hasLocalizedManyNumberField to something more general to include the text field as well.

Cuzart avatar Dec 28 '23 11:12 Cuzart

I already worked on the same issue and I also reviewed your changes. I noticed that in the payload-types.ts the type for the hasMany, validateHasMany and so on is still string | null in my case it was set to string [] | null as I also included some changes in the "buildObjectType", "configToJSONSchema", and "buildMutationInputType". Maybe you want to have a look at my forked repo or give me access to work on the PR so I can also include those changes and the unit test I wrote @Gokulsck

@Cuzart payload-types.ts is auto-generated not modified manually.

Gokulsck avatar Dec 28 '23 15:12 Gokulsck

Sure, I know I did not manually modify it. I have run the generate-types script and modified the utility that is used to generate the file see configToJSONSchema.ts @Gokulsck

Cuzart avatar Dec 28 '23 17:12 Cuzart

Sure, I know I did not manually modify it. I have run the generate-types script and modified the utility that is used to generate the file see configToJSONSchema.ts @Gokulsck

Ok. And the changes for Postgres db adapter is not very straightforward. Could you please check on that?

Gokulsck avatar Dec 29 '23 15:12 Gokulsck

Yes you are right as I got more into it, it is not really straightforward. I am on it and got it working with postgres now but some tests are still failing so I have to do some more work here.

Cuzart avatar Dec 29 '23 19:12 Cuzart

I tested it with postgres running locally all unit and e2e seem to run fine I guess that should be it and ready to be reviewed @DanRibbens . Hope I did not miss anything.

Cuzart avatar Dec 29 '23 19:12 Cuzart

Just a note that it seems you guys have updated the docs before actually creating a new release with these changes. This caused me a bit of confusion when trying to use the hasMany option as currently documented here

maxsynnott avatar Jan 05 '24 19:01 maxsynnott