Pagination for organization view #3490

Learningstuff98 · 2023-03-23T02:49:46Z

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.

Learningstuff98 · 2023-03-23T03:12:18Z

I'll look into the failing tests and see what I can do.

dorner

One comment, in addition to the tests :)

spec/system/admin/organizations_system_spec.rb

Learningstuff98 · 2023-03-24T22:18:29Z

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.

awwaiid · 2023-03-26T03:43:39Z

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.

Learningstuff98 · 2023-03-31T02:09:37Z

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?

cielf · 2023-04-02T16:59:37Z

@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.

seanmarcia · 2023-04-02T17:21:08Z

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 · 2023-04-02T23:17:05Z

@cielf I tried this and unfortunately it didn't work.

cielf · 2023-04-03T00:19:49Z

@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.

dorner · 2023-04-03T00:30:50Z

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.

Learningstuff98 · 2023-04-03T00:58:31Z

@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.

cielf · 2023-04-03T01:15:39Z

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?

Learningstuff98 · 2023-04-03T02:42:26Z

@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.

cielf · 2023-04-03T03:11:46Z

@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...)

cielf · 2023-04-03T03:22:23Z

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.

Learningstuff98 · 2023-04-03T03:29:04Z

@cielf Yeah its messy.

cielf · 2023-04-03T03:34:21Z

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)

cielf · 2023-04-03T03:37:39Z

Bleah. Other tests are relying on that name being DEFAULT.

cielf · 2023-04-03T03:41:27Z

@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.

Learningstuff98 · 2023-04-03T03:53:03Z

@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.

cielf · 2023-04-03T04:03:47Z

@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.

taking over PR

cielf · 2024-04-07T16:59:22Z

spec/system/admin/organizations_system_spec.rb

@@ -1,7 +1,8 @@
-RSpec.describe "Admin Organization Management", type: :system, js: true do
+RSpec.describe "Admin Organization Management", type: :system, js: true, seed_items: false, skip_transaction: true do
+  let(:super_admin) { create(:super_admin) }


I'm wondering if this should be super_admin_no_org.

There really shouldn't be a difference.

In fact we should be changing super_admin to not have an org and fix any specs that get confused by that. Super admins should not be considered to belong to orgs.

I agree. And that's probably a separate issue. g

cielf

Something else I noticed when I kicked the tires is that when you filter down the organizations, you still have the full range of pages at the bottom.
The user pagination shows that same behaviour, but we seem to handle it correctly on the distribution index.

# Conflicts: # app/views/admin/organizations/_list.html.erb

dorner · 2024-04-12T20:34:35Z

@cielf pushed a fix!

Looks good from a functionality pov now

cielf

It passes my manual testing, but it looks like there are tests related to this area that are failing still.

dorner · 2024-04-14T13:44:49Z

I think this might be related to some of the seed stuff that @elasticspoon was working on.

elasticspoon · 2024-04-14T13:48:48Z

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

dorner · 2024-04-14T13:51:36Z

Looks like this is the only spec that has skip_transaction: true. I think this was me trying to get around the seeded data and having all clean data. So this was basically the beta version of what you're doing. :) But it doesn't seem to be interacting well with your changes.

cielf · 2024-04-14T16:19:32Z

Am asking @awwaiid to take a look from a technical pov.

github-actions · 2024-05-23T00:12:20Z

Automatically unassigned after 7 days of inactivity.

cielf · 2024-05-23T18:42:17Z

Have poked @awwaiid for a technical review.

# Conflicts: # spec/rails_helper.rb # spec/system/admin/organizations_system_spec.rb

…zaation-view

awwaiid · 2024-06-19T15:23:56Z

spec/rails_helper.rb

@@ -114,13 +114,6 @@ def self.capybara_tmp_path

  # Make FactoryBot easier.
  config.include FactoryBot::Syntax::Methods
-  config.around(:each, skip_transaction: true) do |example|


Is this removal here on purpose?

Ah, I see -- there are no more specs with skip_transaction mentioned. Got it.

Changes addressed

awwaiid

Great! Small diff in the end (ignoring whitespace). I did some manual testing too.

github-actions · 2024-06-23T14:16:17Z

@Learningstuff98: Your PR Pagination for organization view is part of today's Human Essentials production release: 2024.06.23.
Thank you very much for your contribution!

Learningstuff98 added 4 commits March 20, 2023 17:30

Added pagination for the admin organization index page

55bf2d7

Added some tests

b3654d6

Changed the pagination threshold

a36acc6

Merge branch 'main' into pagination-for-organizaation-view

0b59a9b

dorner previously requested changes Mar 23, 2023

View reviewed changes

spec/system/admin/organizations_system_spec.rb Show resolved Hide resolved

Added some tests

1a2de0b

Learningstuff98 added 4 commits March 28, 2023 15:41

Updated some tests

a51cbef

Updated the request_system_spec

257af79

Updated user_system_spec

e9ef93c

Updated a test

a985cb0

dorner requested a review from cielf April 5, 2024 21:04

dorner self-assigned this Apr 5, 2024

cielf reviewed Apr 7, 2024

View reviewed changes

cielf previously requested changes Apr 9, 2024

View reviewed changes

dorner added 2 commits April 12, 2024 16:33

Fix pagination

1ba3fca

Merge branch 'main' into pagination-for-organizaation-view

0846bab

# Conflicts: # app/views/admin/organizations/_list.html.erb

Fix failing specs

7b89eec

cielf previously requested changes Apr 13, 2024

View reviewed changes

Spec fixes

7156499

cielf requested a review from awwaiid April 14, 2024 16:19

dorner mentioned this pull request Apr 17, 2024

refactor: change item seeding removal strategy #4264

Merged

github-actions bot added the stale label May 15, 2024

github-actions bot unassigned dorner May 23, 2024

dorner and others added 2 commits May 24, 2024 16:07

Merge branch 'main' into pagination-for-organizaation-view

934c59b

# Conflicts: # spec/rails_helper.rb # spec/system/admin/organizations_system_spec.rb

Merge remote-tracking branch 'origin/main' into pagination-for-organi…

f2c528a

…zaation-view

awwaiid reviewed Jun 19, 2024

View reviewed changes

awwaiid approved these changes Jun 19, 2024

View reviewed changes

awwaiid merged commit 4cf8899 into main Jun 19, 2024
38 checks passed

awwaiid deleted the pagination-for-organizaation-view branch June 19, 2024 16:12

Pagination for organization view #3490

Pagination for organization view #3490

Conversation

Learningstuff98 commented Mar 23, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Screenshots

Learningstuff98 commented Mar 23, 2023

dorner left a comment

Choose a reason for hiding this comment

Learningstuff98 commented Mar 24, 2023

awwaiid commented Mar 26, 2023

Learningstuff98 commented Mar 31, 2023

cielf commented Apr 2, 2023

seanmarcia commented Apr 2, 2023

Learningstuff98 commented Apr 2, 2023

cielf commented Apr 3, 2023

dorner commented Apr 3, 2023

Learningstuff98 commented Apr 3, 2023

cielf commented Apr 3, 2023

Learningstuff98 commented Apr 3, 2023

cielf commented Apr 3, 2023 • edited Loading

cielf commented Apr 3, 2023

Learningstuff98 commented Apr 3, 2023

cielf commented Apr 3, 2023 • edited Loading

cielf commented Apr 3, 2023

cielf commented Apr 3, 2023 • edited Loading

Learningstuff98 commented Apr 3, 2023 • edited Loading

cielf commented Apr 3, 2023

cielf Apr 7, 2024

Choose a reason for hiding this comment

dorner Apr 12, 2024

Choose a reason for hiding this comment

dorner Apr 12, 2024

Choose a reason for hiding this comment

cielf Apr 12, 2024

Choose a reason for hiding this comment

cielf left a comment

Choose a reason for hiding this comment

dorner commented Apr 12, 2024

cielf left a comment

Choose a reason for hiding this comment

dorner commented Apr 14, 2024

elasticspoon commented Apr 14, 2024

dorner commented Apr 14, 2024

cielf commented Apr 14, 2024

github-actions bot commented May 23, 2024

cielf commented May 23, 2024

awwaiid Jun 19, 2024

Choose a reason for hiding this comment

awwaiid Jun 19, 2024

Choose a reason for hiding this comment

awwaiid left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 23, 2024

Learningstuff98 commented Mar 23, 2023 •

edited

Loading

cielf commented Apr 3, 2023 •

edited

Loading

cielf commented Apr 3, 2023 •

edited

Loading

cielf commented Apr 3, 2023 •

edited

Loading

Learningstuff98 commented Apr 3, 2023 •

edited

Loading