Superadmin Partners View (4682) & Category Dropdown Error (4674) #4703

bdeanhardt · 2024-10-03T14:18:46Z

Description

The super admin partners view is now in alphabetical order.
If you have an error on new item, the category list for selection will not disappear anymore.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I reopened the super admin page after implementing my changes and the partners are now in alphabetical order.
I tried to reproduce the bug after implement my bug fix and I was unable to do so (categories will show up now despite an error).
Everything else still works as expected.

Screenshots

cielf · 2024-10-03T18:13:24Z

Hey @bdeanhardt -- Thank you! -- we'll take a look. One thing for future -- we usually put a PR for each issue we work on -- it's easier for the testers.

cielf · 2024-10-04T18:06:17Z

Also for future -- it's a better practice to make a branch, rather than making the changes in your main.

cielf · 2024-10-04T18:17:29Z

@bdeanhardt Could you make the order "human alphabetical" -- i.e. case insensitive, aka lower case alphabetical?, rather than "computer alphabetical", which orders all the lowercase after all the upper case, please?

(We're in the process of making the order of all the other alphabetical lists case-insensitive)

Thanks,

cielf

Hi @bdeanhardt
Welcome aboard! Thanks for your work on this!

The item fix works very well!

For the partner page, I'm afraid we had a slight misunderstanding on what we mean by alphabetical -- we want alphabetical the way English-speaking humans do it -- i.e. both 'A' and 'a' are treated the same.

We'll also need automated tests for both of these. That's what we mean when we need tests.

Please see the other procedural comments I've made, above, for future pull requests.

cielf · 2024-10-09T14:58:19Z

@bdeanhardt Functionally, this looks great! -- just waiting for those automated tests, then I'll pass it over to our tech lead for a final technical review.

dorner

Can you add or modify an existing test to validate these fixes?

bdeanhardt · 2024-10-16T18:52:35Z

Can you add or modify an existing test to validate these fixes?

tests are in! Thank you for your patience on this.

dorner

One comment on the newly added test. Shouldn't there be one more added test since there are two issues being fixed here?

dorner · 2024-10-22T13:47:55Z

spec/controllers/items_controller_spec.rb

+          post :create, params: bad_params
+
+          expect(response).to render_template(:new)
+          expect(assigns(:item_categories)).to eq(organization.item_categories.order('name ASC'))


This statement is just restating the code itself. You need to create item categories with specific names and validate that the printed categories match those names in alphabetical order.

Should I check for the alphabetical order of the categories in both tests? Or is this OK as is now that the first test is pushed?

Actually, I got confused - this test doesn't actually test the bug, which is that the categories were empty after an error. You should do this in a request test which validates the output HTML.

Ok! I uploaded a new test.

bdeanhardt · 2024-10-22T14:34:52Z

One comment on the newly added test. Shouldn't there be one more added test since there are two issues being fixed here?

I forgot to push the other test! I'm so sorry totally my bad.

dorner · 2024-10-23T00:38:35Z

spec/controllers/admin/partners_controller_spec.rb

+
+        it "assigns partners ordered by name (case-insensitive)" do
+          get :index
+          expect(assigns(:partners)).to eq([partner1, partner2, partner3].sort_by { |p| p.name.downcase })


Suggestion:

expect(assigns(:partners).map(&:name)).to eq(%w(alpha Bravo Zeus))

But if you really want to test this, you should swap the names of partners 1 and 2. This way the default order would not be alphabetical and you'd actually be testing that the sort works. 😄

dorner · 2024-10-23T00:39:47Z

spec/controllers/items_controller_spec.rb

+          post :create, params: bad_params
+
+          expect(response).to render_template(:new)
+          expect(assigns(:item_categories)).to eq(organization.item_categories.order('name ASC'))


Actually, I got confused - this test doesn't actually test the bug, which is that the categories were empty after an error. You should do this in a request test which validates the output HTML.

dorner

Almost there!

dorner · 2024-10-27T21:30:07Z

spec/requests/items_requests_spec.rb

+        expect(response).to render_template(:new)
+
+        # Ensure the item categories are assigned in the controller
+        expect(assigns(:item_categories)).to eq(organization.item_categories.order('name ASC'))


I much prefer giving the categories explicit values and testing against those values rather than pulling them from the DB. You end up basically just copying the code you're testing into the test.

Sorry, I'm having trouble understanding exactly what you mean here.

I mean doing something like

let(:categories) do [ FactoryBot.create(:item_category, name: 'Apples'), # include whatever other IDs need to be passed in FactoryBot.create(:item_category, name: 'Bananas') ] end # ... expect(assigns(:item_categories)).to eq(categories))

Hey @bdeanhardt -- do you need further guidance on this?

Hey @cielf! Past 2 weeks have been chaos, but I'm back! I'm taking a look today :)

So sorry about the delay! How does it look now?

You shouldn't need to specify an order in the spec.

To be a bit more directive, here's what I had in mind:

let!(:category1) { FactoryBot.create(:item_category, name: 'Bananas', organization: organization) } let!(:category2) { FactoryBot.create(:item_category, name: 'Apples' organization: organization) } let!(:category3) { FactoryBot.create(:item_category, name: 'Umbrella', organization: organization) } # ... expect(assigns(:item_categories)).to eq([category2, category1, category3])

dorner · 2024-10-27T21:30:29Z

Lint also needs to be fixed.

Addressed

dorner · 2024-11-22T01:18:47Z

spec/controllers/admin/partners_controller_spec.rb

+
+      it "assigns partners ordered by name (case-insensitive)" do
+        get :index
+        expect(assigns(:partners)).to eq([partner1, partner2, partner3].sort_by { |p| p.name.downcase })


You shouldn't need to sort since you know the partners' names. :) Just reorder the array to partner2, partner1, partner3.

Actually... can you switch it so the partner1line is moved above the partner2 line? That way they'd actually be created out of order. The order of the let! statements is what matters.

dorner · 2024-11-22T01:19:31Z

spec/requests/items_requests_spec.rb

+
+      before do
+        categories  # Ensure categories are created before the request
+      end


You can remove this before block and replace the let with let! to ensure they are created.

dorner · 2024-11-22T01:21:15Z

spec/requests/items_requests_spec.rb

+        expect(response).to render_template(:new)
+
+        # Ensure the item categories are assigned in the controller
+        expect(assigns(:item_categories)).to eq(organization.item_categories.order('name ASC'))


You shouldn't need to specify an order in the spec.

To be a bit more directive, here's what I had in mind:

let!(:category1) { FactoryBot.create(:item_category, name: 'Bananas', organization: organization) } let!(:category2) { FactoryBot.create(:item_category, name: 'Apples' organization: organization) } let!(:category3) { FactoryBot.create(:item_category, name: 'Umbrella', organization: organization) } # ... expect(assigns(:item_categories)).to eq([category2, category1, category3])

dorner · 2024-11-22T01:22:01Z

Both tests and lint are still failing. :(

bdeanhardt added 2 commits October 3, 2024 10:13

sorting partners by name

a3fa8d6

bug fix for category dropdown menu dissappearance

c1aeb5c

bdeanhardt changed the title ~~4682 - Superadmin Partners View~~ Superadmin Partners View (4682) & Category Dropdown Error (4674) Oct 3, 2024

bdeanhardt mentioned this pull request Oct 3, 2024

[BUG] On new item, category dropdown disappears after error #4674

Open

2 tasks

cielf previously requested changes Oct 4, 2024

View reviewed changes

updated partners to be sorted in human alphabetical order

ab06f12

dorner requested changes Oct 10, 2024

View reviewed changes

updated to include tests

12bed2e

dorner requested changes Oct 22, 2024

View reviewed changes

test for alphabetical order

8583e34

dorner requested changes Oct 23, 2024

View reviewed changes

bdeanhardt added 4 commits October 24, 2024 11:44

fixed alphabetical order testing and removed incorrect controller test

4fd0140

small fix

186fc3b

small fix

a67431a

updated test for categories bug fix

a65d019

dorner requested changes Oct 27, 2024

View reviewed changes

bdeanhardt added 2 commits October 29, 2024 09:18

linter fix

20f4ba5

updated tests to manually create categories

94d4728

dorner requested changes Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Superadmin Partners View (4682) & Category Dropdown Error (4674) #4703

Superadmin Partners View (4682) & Category Dropdown Error (4674) #4703

bdeanhardt commented Oct 3, 2024 •

edited

Loading

cielf commented Oct 3, 2024 •

edited

Loading

cielf commented Oct 4, 2024

cielf commented Oct 4, 2024

cielf left a comment

cielf commented Oct 9, 2024

dorner left a comment

bdeanhardt commented Oct 16, 2024

dorner left a comment

dorner Oct 22, 2024

bdeanhardt Oct 22, 2024

dorner Oct 23, 2024

bdeanhardt Oct 24, 2024

bdeanhardt commented Oct 22, 2024

dorner Oct 23, 2024

dorner Oct 23, 2024

dorner left a comment

dorner Oct 27, 2024

bdeanhardt Oct 29, 2024

dorner Oct 30, 2024

cielf Nov 11, 2024

bdeanhardt Nov 12, 2024

bdeanhardt Nov 19, 2024

dorner Nov 22, 2024

dorner commented Oct 27, 2024

dorner Nov 22, 2024

dorner Nov 22, 2024

dorner Nov 22, 2024

dorner commented Nov 22, 2024

Superadmin Partners View (4682) & Category Dropdown Error (4674) #4703

Are you sure you want to change the base?

Superadmin Partners View (4682) & Category Dropdown Error (4674) #4703

Conversation

bdeanhardt commented Oct 3, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Screenshots

cielf commented Oct 3, 2024 • edited Loading

cielf commented Oct 4, 2024

cielf commented Oct 4, 2024

cielf left a comment

Choose a reason for hiding this comment

cielf commented Oct 9, 2024

dorner left a comment

Choose a reason for hiding this comment

bdeanhardt commented Oct 16, 2024

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdeanhardt commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner commented Oct 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner commented Nov 22, 2024

bdeanhardt commented Oct 3, 2024 •

edited

Loading

cielf commented Oct 3, 2024 •

edited

Loading