Closed Adonaioc closed 5 years ago
@Adonaioc Actually, I was able to fix the issue with the build related to permissions. It looks like the unit tests are now legitimately failing. Can you please check why the tests are failing? You can view the output here: https://travis-ci.org/coldbox-modules/cbox-cborm/builds/170812077
Any updates on this?
I think we need some tests around the functionality.
So I have to admit I have not used the unit testing capabilities of your coldbox project before and am having a bit of trouble getting it to run locally, do I need to install something else to make it work?
Have you followed the instructions here? https://github.com/ColdBox/coldbox-platform/blob/development/tests/readme.md
I got the tests running locally and it looks like only one is failing and its because I changed the return type of createDetachedSQLProjection from sqlProjection to org.hibernate.criterion.AliasedProjection which implements EnhancedProjection which extends projection. The reason for this is to allow sorting on detached criteria, Should I modify the test to check the new type or create a separate function for this with a new test or something entirely different?
@Adonaioc : I see this PR is hanging out there. I would suggest just modifying the return type check on the test, unless @lmajano has any objections.
Thanks! I no longer work with cf or coldbox but I love what you are doing in the community!
Sent from my iPhone
On Jul 1, 2019, at 11:13 AM, Luis Majano notifications@github.com wrote:
@lmajano commented on this pull request.
In modules/cborm/models/criterion/Restrictions.cfc:
@@ -169,8 +169,21 @@ component singleton{ return restrictions.sizeNE(arguments.property, arguments.propertyValue); }
- // Use arbitrary SQL to modify the resultset
- any function sqlRestriction(required string sql){
- any function sqlRestriction(required string sql, any parameters ){
- //if we defined a parameter then its assume its a string and pass it through right I have merged this manually into be, but have added full support for all types.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
It might have been part of something for another modification to the module that was not fully included in the pull request. We extended the orm module quite a bit. I no longer have access to the codebase but I don’t see how it would be applicable by itself in this context.
On Jul 1, 2019, at 4:56 PM, Luis Majano notifications@github.com wrote:
@lmajano commented on this pull request.
In modules/cborm/models/BaseBuilder.cfc:
@@ -557,8 +557,8 @@ component accessors="true"{ var partialSQL = ""; projection.sql = ""; // if multiple subqueries have been specified, smartly separate them out into a sql string that will work
- for( var x=1; x<=listLen( arguments.rawProjection.sql ); x++ ) {
- partialSQL = listGetAt( arguments.rawProjection.sql, x );
- for( var x=1; x<=listLen( arguments.rawProjection.sql, "&&" ); x++ ) { I am not sure I follow why the adding of the && can you refresh my memory why?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Thanks so much @Adonaioc , I will be releasing a major update to the module this week. Thanks for the feedback and too bad you are not using ColdBox anymore. Don't forget about us and send us features or ideas 👍
https://github.com/coldbox-modules/cborm/blob/development/changelog.md
@Adonaioc I needed to make a fix for the permissions on the Travis build. Can you please rebase your branch against
development
to get those commits. Then we can see if your pull passes.