aesmail / kaffy

Powerfully simple admin package for phoenix applications
https://kaffy.fly.dev/admin/
MIT License
1.34k stars 155 forks source link

Dynamic Primary Keys #270

Closed nullpilot closed 1 year ago

nullpilot commented 1 year ago

This is the refactored version of #151, which addresses the hard-coded id primary key, and issues discussed in #6.

The first two commits are the same as before, with merge conflicts resolved. The rest are to implement list actions and address some of what was discussed in the linked issue, hoping to make this change a less rough around the edges. ๐Ÿ˜ƒ

The default serialization method remains unchanged from the old PR, but the separator was changed to ":", and an example on how the serialization could be done with ETF instead, for cases where the former method may introduce ambiguity, was added to the docs.

For the list actions, I split the behavior as follows:

Happy to hear your thoughts!

aesmail commented 1 year ago

Amazing work @nullpilot !

I feel like this feature deserves more than a "patch" version increment ๐Ÿ˜„

If you could just add tests for it, this would be merged as soon as all tests are passing ๐Ÿ™๐Ÿผ

nullpilot commented 1 year ago

I fixed the tests and added a few for the green path. Ideally I would like to improve error handling some, but it didn't do too much for that yet. Another change I considered but haven't added to this PR, is to make the default deserialization cast back to the schema type. It felt a bit too fickle, and Ecto handles it further down the pipeline anyway, probably with better error handling.