Skip to content
This repository has been archived by the owner on Dec 27, 2024. It is now read-only.

Return the proper full count from DB queries #496

Merged

Conversation

grantila
Copy link
Contributor

@grantila grantila commented Nov 20, 2024

Summary 📰

This fixes #495 and effectively reverts #464 which is incorrect.

The UI uses the count field to build the navigation between pages, to know how many pages exist, and allow traversing them. #464 broke this, by returning count as the number returned entries (an unnecessary value really - .length would suffice for that).

The PR also updates and fixes the tests to reflect this.

The DB logic was slightly refactored to be easier to understand, by having two filter functions (filterState() and filterRange()), both being used by the main result query, but only the first being used for the count query. This results in a proper count value.

The changeset for #464 says:

For example, if there are 2 total announcements in the database, and you request a max of 1 announcement, the count would still return 2.

This was actually correct. If you only want 1 announcement, it makes perfect sense to return the total number of announcements (i.e. 2) as count. It's also necessary for pagination.

Checklist ✅

  • I have updated the necessary documentation
  • I have signed off all my commits as required by DCO
  • My build is green

Testing Notes 🔬

To reproduce; Have more announcements than shown in the main page (first page). The bottom page navigation doesn't work. It does with this PR.

@grantila grantila requested review from kurtaking and a team as code owners November 20, 2024 13:49
Copy link

changeset-bot bot commented Nov 20, 2024

🦋 Changeset detected

Latest commit: 50a6b2a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@procore-oss/backstage-plugin-announcements-backend Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@kurtaking kurtaking left a comment

Choose a reason for hiding this comment

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

Hey @grantila, thank you for the contribution. ❤️ Learned a few things from this. Thank you for cleaning up my mistake 😄

@kurtaking kurtaking merged commit 11cacf9 into procore-oss:main Nov 25, 2024
3 checks passed
@grantila
Copy link
Contributor Author

Glad I could help @kurtaking, I see there are some errors in the publishing: https://github.com/procore-oss/backstage-plugin-announcements/actions/runs/12002387031/job/33454275718 making it not end up on npmjs 🤔

@grantila
Copy link
Contributor Author

grantila commented Dec 2, 2024

@kurtaking, would you please make a new release with this fix?

@kurtaking
Copy link
Member

@kurtaking, would you please make a new release with this fix?

@grantila - token was expired. I'm working through fixing now.

@kurtaking
Copy link
Member

@grantila - try again. Also, my plan is to migrate the announcements plugins to backstage/community-plugins this week. Using their CI and release process will be a more stable process, and migrating to that org will allow for faster contributions.

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

Successfully merging this pull request may close these issues.

🐛 Bug Report: Pagination always displays only one page, even when there are more than 10 announcements
2 participants