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

Fix report search for "pending approval of" #3445

Merged
merged 5 commits into from
Feb 24, 2021

Conversation

gjvoosten
Copy link
Collaborator

Make sure the Home tile for "Reports pending my approval", and report search results for "pending approval of", show the correct number of results.

Closes #3442

User changes

  • The Home tile for "Reports pending my approval", and report search results for "pending approval of", will show the correct number of results again.

Super User changes

  • none

Admin changes

  • none

System admin changes

  • anet.yml or anet-dictionary.yml needs change
  • db needs migration
  • documentation has changed
  • graphql schema has changed

Checklist

  • Described the user behavior in PR body
  • Referenced/updated all related issues
  • commits follow a repo#issue: Title title format and these 7 rules
  • commits have a clean history, otherwise PR may be squash-merged
  • Added and/or updated unit tests
  • Added and/or updated e2e tests
  • Added and/or updated data migrations
  • Updated documentation
  • Resolved all build errors and warnings
  • Opened debt issues for anything not resolved here

@gjvoosten gjvoosten added the bug label Feb 17, 2021
@lgtm-com
Copy link

lgtm-com bot commented Feb 17, 2021

This pull request introduces 2 alerts when merging 8eb6ac5 into 4adb2b1 - view on LGTM.com

new alerts:

  • 2 for Exposing internal representation

@gjvoosten gjvoosten marked this pull request as ready for review February 17, 2021 16:13
@gjvoosten gjvoosten force-pushed the GH-3442-fix-pendingApproval branch from a56dc76 to 6a6b1ee Compare February 24, 2021 07:39
gjvoosten and others added 5 commits February 24, 2021 08:40
Since this test is using a default pageSize of 10, it will succeed.
This is what's used for the Home tile of "Reports pending my approval".
The test will fail now, since the code has not yet been fixed.
…ng approval of someone

Since for reports searched by pendingApprovalOf we have to do some (difficult) post-processing,
first query the database for all reports, then post-process them,
and finally slice the requested page from the remaining results.
Thus we get an accurate totalCount and correctly filled pages of results.
It means we're retrieving more results rows than we actually need (and return),
but since users don't normally have a huge amount of reports pending their approval,
this is acceptable.
@gjvoosten gjvoosten force-pushed the GH-3442-fix-pendingApproval branch from 6a6b1ee to 1054bb5 Compare February 24, 2021 07:40
@VassilIordanov VassilIordanov merged commit bd74827 into candidate Feb 24, 2021
@VassilIordanov VassilIordanov deleted the GH-3442-fix-pendingApproval branch February 24, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard "Reports pending my approval" counter is stuck at 1
4 participants