search

SoftwareBrothers / adminjs-sequelizejs

Sequelizejs adapter for AdminBro
MIT License
27 stars 34 forks source link

fixes #26 - resources.find now checks if the sortBy is a typeof VIRTUAL #27

Closed MichaelLeeHobbs closed 4 years ago

MichaelLeeHobbs commented 4 years ago

fixes #26 - resources.find now checks if the sortBy is a typeof Sequlize.VIRTUAL and throws an error that it cannot sort by a typeof VIRTUAL and how to fix the issue

MichaelLeeHobbs commented 4 years ago

We should add regression tests but that is not something I have time for now. At the earliest, it would be a week or more before I have time for that.

MichaelLeeHobbs commented 4 years ago

Test added. Also committed changed to make dev work on Windows. I would have done this as a separate pull request but GitHub automatically added the commit to this pull request.

wojtek-krysiak commented 4 years ago

Hi @MichaelLeeHobbs - thanks for the PR. I was looking and it and I think that the simplest solution would be to disable the sorting for virtual properties instead of throwing errors. (in property override isSortable that it returns false for virtual properties).

Otherwise, it is a risk that this error will be thrown on production because the developer who implemented admin-bro simply didn't know about this and tester didn't click "sort" button.

What do you think?

MichaelLeeHobbs commented 4 years ago

I agree. I just hadn't got back around to updating this. Been a very busy couple of weeks. There are some other items I'd like to address in admin-bro-sequelizejs but for not I think the making the virtuals not sortable is the best solution.

MichaelLeeHobbs commented 4 years ago

I will do a clean fork and put the other changes into a new pull request.

wojtek-krysiak commented 4 years ago

hehe - the same here - busy 2 weeks. So I am closing this one for now.