rubyforgood
Pagination for organization view
Resolves #3471
Description
This PR adds pagination to the super admin organization index page.
List any dependencies that are required for this change. (gems, js libraries, etc.)
kaminari, Rspec, FactoryBot
Type of change
- New feature (non-breaking change which adds functionality)
How Has This Been Tested?
Visual confirmation and I wrote a system test.
Screenshots
I set the pagination to one item per page so I could take these screenshots. This PR does not change the amount of items per page.
If anything needs to change, please let me know.
Unfortunately I seem to have an extra organization instance making its way into the test file that I'm working on, called DEFAULT. I've looked through the app to see if I can find the culprit but I haven't found anything so far. If anyone has any ideas please let me know.
Looks like https://github.com/rubyforgood/human-essentials/blob/main/spec/rails_helper.rb#L157 adds this DEFAULT organization during test setup, so your test should assume that it exists.
I have an update. I'm down to two tests in the organizations_system_spec. The one that's failing on line 77 has me stumped and I'm wondering if this is actually a bug with rspec. The search works fine in the browser. Here's a screenshot provided by rspec.
The one that's failing on line 114 is passing locally for me and works in localhost. Is there a way to access the screenshots that rspec takes on github?
@Learningstuff98 I don't think the one at 77 is a bug with rspec. The name of the user in this case is "DEFAULT SUPERADMIN", and the name of the organization you are looking for is "DEFAULT.
I would add a different bank with a nonsense name to be the one that I am checking isn't there.
I just ran the tests locally and got errors withorganizations_system_spec.
When running the test suite locally and trying to reproduce errors I recommend running the entire test file in case something is wrong in the build-up to the specific test. If you're still green locally, and there are errors on CI you should run it with the same rspec seed as on CI and see if that produces any errors -- it could be that a flakey test has been introduced or already there and CI caught it.
@Learningstuff98 Yeah - it didn't work for me either - I'm still getting the tests passing on local. I stand by my recommendations re the bank name, though.
If an individual test is passing, often it means that it's the suite that's failing - meaning that some spec is not cleaning up after itself correctly, causing a later spec to fail. If you grab the seed that ran and run the same command that ran on GitHub Actions, you're more likely to see the failure. You can use --fail-fast so it stops with the first failure.
@cielf I should have clarified, I meant your recommendation about the bank name for line 77. Its as if the search isn't running before the expect statements execute or it isn't running at all.
Hmmm... @Learningstuff98 Are you saying that once you use another bank with a nonsense name, instead of Organization.first to do the check, it's now failing on your local?
@cielf Setting the foo_org variable like the following:
let!(:foo_org) { create(:organization, name: 'foo') }
Passes for line 77, however other assertions in the "filters by organizations by name in organizations index page" it statement to fail. Line 86 ends up failing in the same way with the search seeming to not execute.
@Learningstuff98 Played around with it a bit -- If you look at line 69-ish It seems like what's happening is it's not showing the last alphabetically? Which would normally speak to me of a loop that's cutting out before it should somewhere. (if you change "foo" to "aoo", then it's "baz" that isn't showing...)
Right... and it's losing the last alphabetically because for testing the # of things on the page is set to 3, right? Right, and you end up with that Default as well as what you are actauly working with.
Having all the defaults have the word "DEFAULT" in them is where the problem "really" is here, IMO. If you go back to before my initial suggestion before the bank, then go to rails_helper.rb and change the DEFAULT_TEST_ORGANIZATION_NAME to, say, "FIRST TEST ORGANIZATION" (which also doesn't have the text "ba" in it), what happens? (By here, I mean with around line 77)
@Learningstuff98 Well, it's terribly terribly kludgey, and I don't like it one bit, but what if your nonsense bank names are all before "DEFAULT", and we'll put in a technical debt issue to clean up other tests relying on the Default Organization name being DEFAULT.
@cielf "what if your nonsense bank names are all before "DEFAULT" " I'd have to look into this tomorrow. But I do want to ask, is it worth it to test pagination here? It seems like this is turning into a bit of a tornado as far as the tests are concerned.
@Learningstuff98 I think we should test the pagination. The flaw in the setup of defaults is a separate newly discovered issue that has been colliding with this.
@cielf For The question "what if your nonsense bank names are all before "DEFAULT"?", it looks like line 69 would pass. As for the other assertions that are looking for a search result, I don't think that the order would make a difference given that the search seems to not be executing or the assertions are executing before the search has a chance to run.
@cielf I tested changing the name of the default org to "FIRST TEST ORGANIZATION" and got the same result for line 77.
@cielf Unfortunately I haven't been able to make any fixing changes so I haven't sent anything. I tried adding things like "sleep 0.5" but that didn't work for line 77.
@cielf I've continued to try to solve this, but unfortunately I think I've hit the end of what I can do here.
@LearningStuff98. S'alright - one of us can take it forward. I'll add it to my mental list of TTD.
I think this might be related to some of the seed stuff that @elasticspoon was working on.
I think this might be related to some of the seed stuff that @elasticspoon was working on.
Could be. I dont remember working to get this spec ready to skip_seed. It should work default to normal behavior if you drop the skip_seed tho
