search

progress / JSDO

Client side TypeScript library to access Progress® Data Object Services
Other
23 stars 27 forks source link

JSDO not adding tableRef into filter #234

Closed chunnybryan closed 5 years ago

chunnybryan commented 5 years ago

HI

We have noticed that even though we supply a tableRef property to a DataSource object in our Angular application the JSDO does not add the tableRef into the filter when calling the count invoke or the read / get.

This causes incorrect results or errors for BE's which have multiple tables.

Code example image

We are using the JSDO version 6 for Angular with Angular 6.

Thanks

Christian.

edselg commented 5 years ago

Hello Christian,

Thank you for reporting the issue.

We will take a look and let you know our findings.

Thank you and regards, Edsel

chunnybryan commented 5 years ago

Hi Edsel

It looks like an addition by Mike Fechner made in JSDO 4.2 on 16.03.2016 has been lost in future JSDO versions.

Can we get this change added back into the JSDO?

Its in the request mapping function (See lines annotated with CJB:

// Christian Bryan - 10.02.2019 Add this back in

// Mike Fechner, Consultingwerk Ltd. 16.03.2016

           // Adding the tableRef property of the JSDO Parameters to

           // the Filter Parameter so that the backend can use this

           // information to actually know which Business Entity Table

           // the query filter string is intended for ...

filter = JSON.stringify({

ablFilter: ablFilter,

tableRef: params.tableRef, // CJB

   viewTables: jsdo.viewTables, // CJB

                                   sqlQuery: sqlQuery,

orderBy: sortFields,

skip: params.skip,

top: params.top

});

Thanks

Christian.

edselg commented 5 years ago

@chunnybryan CC: @consultingwerk

Hello Christian,

I do not think that the enhancement with viewTables was ever submitted as a PR to the JSDO.

We can certainly merge the change to include tableRef and viewTables for the filter.

Mike, could you create a PR for the develop branch with this enhancement?

Thank you and regards, Edsel

chunnybryan commented 5 years ago

HI I am going to create a local branch with this change in and push it up to the REPO so i can make PR. When i try to PUSH my local branch to the REPO i get a permission error does someone need to give me permission to the REPO? Thanks Christian.

chunnybryan commented 5 years ago

image

edselg commented 5 years ago

Hello Christian,

Thank you for working on this change.

I think that the right process for this would be to fork the JSDO repo and implement the change in your own repo, then you would be able to submit it as a PR.

Related links:

Thanks again.

chunnybryan commented 5 years ago

HI Edsel

Okay got it. The PR should be there now.

Thanks

edselg commented 5 years ago

Thanks a lot, Christian.

I see the PR. I changed it to merge into the develop branch. It shows some additional diff but I should be able to merge it.

I hope to merge it soon.

Thanks again, Edsel

edselg commented 5 years ago

Internal issue # ADAS-15442.

edselg commented 5 years ago

@chunnybryan @consultingwerk

Hello Christian,

Thank you for submitting this issue and the pull request.

I have merged the pull request and also added a unit test program.

You can run the unit test by using the following steps:

I noticed that you had not included the initialization of viewTables. I think that is fine so that viewTables would be an optional property which can be undefined or initialized to a desired value.

This change should be available via npmjs.com in a future release.

Thanks again, Edsel

edselg commented 5 years ago

Pull requests: