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

Add support for giving back total count via header #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkg20001
Copy link

This adds a new includeTotalCount flag that by default exposes the current range and total count as specified in content-range and a property totalCountHeader, which if set to anything other than 'content-range' will add a header that contains just the total number of elements

This is more or less an attempt to be compatible with whatever https://www.npmjs.com/package/ra-data-simple-rest expects as return value

Note: this is incomplete as it's missing support in other tandys with count support

Copy link
Member

@mattboutet mattboutet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd be willing to flesh this out further to hit the other spots with count support and add documentation to the README, I'd be happy to merge this.

lib/actions/find.js Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 25, 2023

Pull Request Test Coverage Report for Build 102

  • 51 of 55 (92.73%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.3%) to 98.611%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/actions/populate.js 13 17 76.47%
Totals Coverage Status
Change from base Build 99: -1.3%
Covered Lines: 284
Relevant Lines: 288

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 101

  • 30 of 30 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 99.861%

Totals Coverage Status
Change from base Build 99: -0.03%
Covered Lines: 718
Relevant Lines: 719

💛 - Coveralls

@mkg20001
Copy link
Author

mkg20001 commented Feb 25, 2023

hrm... in populate modelFull[options.associationAttr] is used to get the length but later modelFull is returned. Does it make sense to return the full object? I mean only the association (the array of items) is/should be populated.

That way it's easier to work with the data, since just the array is returned. Esp since we also do pagination, that seems odd.

Then it might make more sense to rewrite the query so it just uses the base object for the join but doesn't select anything from it.

@mkg20001
Copy link
Author

mkg20001 commented Feb 25, 2023

I have some ideas on populate: Currently it's only possible to populate one specific key. The user may want to populate some or all of them. And for a particular object. So it may make sense to provide a query parameter for find-by-id (or even find aswell) where the user can specify "*" or a comma-seperated list of association attributes to populate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants