Angus Ryer (f692349c) at 17 Mar 18:14

Angus Ryer (ea5ff66b) at 17 Mar 18:14

Angus Ryer (e69447d3) at 17 Mar 18:14

Angus Ryer (cef87f96) at 17 Mar 18:14

Angus Ryer (a96351e1) at 17 Mar 18:14

Angus Ryer (21de66dd) at 17 Mar 13:29

... and 272 more commits

@lindsey-shelton Ah yes, this seems really obvious now 😆 -- We'll have to let this MR sit a bit while the team does a refactor around it, but I'll make this change if/when I come back to it. Thank you!

@justin_ho Yep that works! My thought here was that it might reduce the cognitive load during refactor by aligning the naming, but I'll move onto the next priority instead. If it happens to become confusing because of the naming, we can revisit. 🙏🏻

@jannik_lehmann Thank you so much for putting this together. I've spent some time reviewing and have left some comments I'd love to hear your perspective on!

First, missing scenarios

Explore namespace

• Private item + enabled (in owner project) === inactive "Enabled" button
• Private item + not enabled === active "Enable" button

Project namespace:

• Private item + enabled item (in its owning namespace) === inactive "Enabled" button
• Private item + not enabled + user is consumer admin === active "Enable" button
• Private item + not enabled + no permissions === no buttons

Test duplication (intentional?)

It looks like about a dozen of these tests have direct or close equivalents in the existing unit specs--in particular the RBAC button visibility tests, which are covered by the describe.each tables in ai_catalog_item_actions_spec, and the search/feature-flag/redirect tests which have unit equivalents in the page-level specs. Many assert on the same data-testid elements that the unit specs already check with the same permission combinations.

I like this test architecture better, however 😉 --it's 762% easier to read.

Let me know what you think of my comments! 🙇🏻

Question

I realize that this is just the initial route, but do we need this empty catch block? What purpose does it serve?

Question

Are we keeping the groupid and groupPath to keep the shape of these PROVIDEs the same?

Praise

Just... thank you, thank you, thank you. 🙏🏻

Thought

The report button and permission test should work in every namespace, but it's in the group namespace here. Not a wrong thing, but structurally, I wonder if these should either be extracted into their own block, or tested explicitly in each namespace to provide "future us" with validation that it is intended to behave the same way across namespaces.

Suggestion

They way this redirect is being tested is different from edit and duplicate. This guarantees it doesn't land on the new page, but also doesn't guarantee what page it lands on.

Nit There's a part of me that wants to name this mountListPage just for searching the codebase and "instant reasoning" when you glance at this name.

Suggestion

This seems like too narrow a test to be an integration test. I'd rather do two tests on either side of the navigation event:

1. Find the search bar, input some text, and verify the query handler receives the search term.
2. Set up the route with the search query param on render, and verify the search input is pre-populated with the term and the query handler is called with it.

Wdyt?

Suggestion

Probably don't need this since we're not asserting its lack of existence in this test.

Although to be honest in another comment I argue that we should assert all the button states in each test because this tells us in one single place what to expect given a namespace + permission set for an item or consumer. To me, this is the big advantage of using this as a safety net.

Note

I have an MR open that changes this confusing test-id name.

Suggestion

Could we split this file up into tests for each namespace? For one, it's a lot to look at, but also as we ramp up AI usage, these large files are harder to parse within the context window.