Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ServiceUtils.mapPagedAxiosResponse returns rowCount of current result set, not total result set #15

Closed
brandongregoryscott opened this issue Jun 27, 2020 · 3 comments · Fixed by #16
Labels
bug Something isn't working

Comments

@brandongregoryscott
Copy link
Contributor

Looks like this was lost in translation when porting

rowCount on the ServiceResponse is being set from data.resultObject.length (row count of the currently returned set), when it really should be set off of data.rowCount (total result set of the current query) which matches our C# PagedResult model:

https://github.com/AndcultureCode/AndcultureCode.JavaScript.Core/blob/master/src/utilities/service-utils.ts#L138

    return {
        results: new ResultRecord<TRecord[]>(data),
        resultObjects: resultObjects,
        rowCount: data?.resultObject?.length ?? 0, // should be rowCount: data.rowCount,
        status: axiosResponse.status,
    };

https://github.com/AndcultureCode/AndcultureCode.CSharp.Core/blob/master/src/AndcultureCode.CSharp.Core/Models/Errors/PagedResult.cs

        public T                          ResultObject   { get; set; }
        public long                       RowCount       { get; set; }

I think this transformation method is used for all list service calls, whether or not the controller is returning paged results. Maybe it would be best to default it to the current calculated value if data.rowCount is null or undefined.

@brandongregoryscott brandongregoryscott added the bug Something isn't working label Jun 27, 2020
@wintondeshong
Copy link
Contributor

Yikes, yeah this needs fixed. Odd, I pulled it straight from latest 🤷‍♂️

@brandongregoryscott
Copy link
Contributor Author

Might've been some accidental refactoring when I was originally porting it over. I have a fix for it on my fork, I can update or write tests for it and get it up soon 👍

@wintondeshong
Copy link
Contributor

It's all good. The rate at which we are getting code up in these repositories lately is worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants