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

package(mongo) [STORY-1030] Add option to define a customer method in order to pass custom query method (to look at paranoid-deleted items for instance #997

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

leo-scalingo
Copy link
Contributor

  • Add a changelog entry in CHANGELOG.md

Sorry, something went wrong.

@leo-scalingo leo-scalingo self-assigned this Nov 20, 2024
Copy link

Copy link
Contributor

@sc-zenokerr sc-zenokerr left a comment

Choose a reason for hiding this comment

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

This looks good, but I think it misses a bit of documentation and some tests.
I think it is important to define the intended behaviour with regards to pagination and sort order and test for that behaviour. It "may" avoid confusion in the future.
What do you think?

@leo-scalingo
Copy link
Contributor Author

What kind of tests would you add? In a way that the rest of the behavior is not changed, only the query getting the documents, that's why I only tested this. If you have one or two test cases to add I would definitely add them

Thanks for your review and I'll add the missing documentation

@sc-zenokerr
Copy link
Contributor

There are already a test cases for returning the second page:

  • "It should return the second page"
  • "It should return the second page in reverse order"

I would suggest adding equivalent tests when using the custom QueryFunc.

I think this would make it clear how we expect the library to behave with regards to pagination and sort order when using QueryFunc

What do you think?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
… order to pass custom query method (to look at paranoid-deleted items for instance
Previously it wasn't possible to paginate over soft deleted documents
@leo-scalingo leo-scalingo force-pushed the fix/1030/mongo-pagination-custom-query branch from 86d36be to c764b19 Compare November 21, 2024 10:34
@leo-scalingo
Copy link
Contributor Author

What do you think?

https://github.com/Scalingo/go-utils/pull/997/files#diff-e90d6bda755cb9479c9f732f823ac4aa98ebff14bc0bb7c1607d0d9bb5418cd6R115-R120

To me the change is minor enough to not having to redo the whole test matrix.

  • Arguments are used the same way
  • The library is not responsible of the WhereQuery anymore, if I would test more testcases, I would not test this change but document.WhereUnscopedQuery, a user of the library could build a custom QueryFunc which breaks the sorting because it takes the argument but does nothing with it, and then it wouldn't be the responsibility of the library. So I'm not sure it's pertinent to test all options.

Copy link
Contributor

@sc-zenokerr sc-zenokerr left a comment

Choose a reason for hiding this comment

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

LGTM. Nice

@leo-scalingo leo-scalingo merged commit d750436 into master Nov 21, 2024
4 checks passed
@leo-scalingo leo-scalingo deleted the fix/1030/mongo-pagination-custom-query branch November 21, 2024 10:52
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.

None yet

2 participants