-
Notifications
You must be signed in to change notification settings - Fork 4k
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 the org post count issue #19503
Fix the org post count issue #19503
Conversation
Thank you for opening this PR! We appreciate you! For all pull requests coming from third-party forks we will need to A Forem Team member will review this contribution and get back to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to only show published, added the suggestion. Thanks for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this! And thanks for fixing this bug! I have a couple of ideas of how we can make this work a bit better, but we're very much on the right track.
@@ -30,6 +30,16 @@ | |||
end | |||
end | |||
|
|||
it "shows the right amount of articles in sidebar" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need two tests here — the other is a super-set of what's being tested here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just feels more intuitive to have the happy path and the edge-case tested in separate tests. If you think it's unnecessary I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine either way, but you could also condense them into a single example that tests different numbers of posts so it feels less arbitrary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for fixing this bug! I think this is moving in the right direction, I noticed a couple of things that could make it even better.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #19503 +/- ##
===========================================
+ Coverage 58.60% 85.51% +26.90%
===========================================
Files 1163 1155 -8
Lines 29593 25991 -3602
Branches 1887 1832 -55
===========================================
+ Hits 17344 22225 +4881
+ Misses 12062 3568 -8494
- Partials 187 198 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@maestromac any idea why the |
@benhalpern Yup I am addressing that now |
Uffizzi Preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing though - did you sign the CLA when you opened this PR? I was fairly sure I saw that you did, but it's not showing up in the PR checks 🤔 you can check whether you've signed or not here
Yes, I did. It should be visible in the checks (it's the last one): |
* Fix the org post count issue * Add regression specs * Move things around and sort them better * Implement PR review * Revert accidental changes --------- Co-authored-by: Mac Siri <mac@forem.com>
What type of PR is this? (check all applicable)
Description
This pull request is just here to fix #19456. I added a regression spec and also a spec to check if the correct amount is displayed at all. The regression spec should be enough, but I'm happy to write more if that's needed.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
You can run the specific specs that I added by running following commands in your terminal:
The org page should now display the correct number:
UI accessibility concerns?
The accessibility shouldn't be impacted, even if this change is for the UI.
Added/updated tests?
have not been included