search

dchester / epilogue

Create flexible REST endpoints and controllers from Sequelize models in your Express app
846 stars 116 forks source link

fix #129 : adding readOnlyAttributes #131

Closed luckypur closed 8 years ago

wilzbach commented 8 years ago

+1 - looks like a nice feature to me :)

mbroadst commented 8 years ago

@luckypur is there a reason we couldn't use excludeAttributes for the same thing? I thought you were asking for excludeAttributes to not only be excluded from the resulting output, but the input as well. Otherwise LGTM

luckypur commented 8 years ago

@mbroadst Actually exclude and readOnly attributes are entirely different things. Like suppose i rarely want to exclude createdOn, modifiedAt kind of attributes, but they are always read only attributes for sure.

luckypur commented 8 years ago

@mbroadst Any update..?

mbroadst commented 8 years ago

@luckypur hey sorry, crazy weekend :)

I see your point, and this looks good, but could you possibly also add the code path for the update controller and include some basic tests for the functionality?

luckypur commented 8 years ago

@mbroadst I am actually new to node+express, so creating a test case may take some time. And i have observed that setting readOnlyAttributes will disable the flexibility of modifying req.body in hooks. Like suppose i want to set 'modifiedBy' in write.before and want to make('modifiedBy') readOnly also..! current PR will delete the modifiedBy, No matter i am setting it manually or its coming from POST requests. So i think best thing is to remove readOnlyAttributes from req.body in Controllers '_control' method in base so that all hooks are always effective. What do you think...? And yeah..Sometimes its better to be Late ... ;)

luckypur commented 8 years ago

@mbroadst Please have a look at updated PR.

mbroadst commented 8 years ago

@luckypur looks good, thanks for the contribution. I'm going to leave it in an unpublished release for now to let you and potentially others check out the feature, you will be able to get the functionality by changing your version for epilogue from ^0.6.5 to dchester/epilogue#master

mbroadst commented 8 years ago

closes #129