Wizard V2: Custom Repositories refactor

[Andrewgdewar]

• Updated content api usage
• Refactored repositories
• A lot of small changes

One design change:
My pm suggested removing the confirmation modal on deletion during the edit screen, I have commented that out and will ask for feedback from all of you. We believe the warning below should be sufficient.

Also updated the empty state screen:

[app-sre-bot]

Can one of the admins verify this patch?

[regexowl]

Thank you for working on the refactor! The code looks much cleaner ❤️

Looking at this from the user point of view there seems to be considerably more loading as more actions trigger a new API call including search and removing repositories.

This is linked to the question of how many repositories should we initially fetch - previously we fetched all the repositories right from the start which looked a bit more seamless when going through the step. What I mean is this:

• previously the fetching happened at the initialization of the step, but then there was no further loading / skeletons:
  

• now the loading state gets triggered on toggling between all/selected, searching, clearing search input, changing pagination perPage limit and removing a repo
  

Which might not be necessarily bad, but it might be good to discuss which path to take from this point. What would be the advantages of handling the data via a series of API calls?

[regexowl]

As for the feedback to removing the modal - I remember adding it for some reason, but can't really remember what the reason was 😅

I guess it was there to make double sure that the user knows they can break their image with the change? Not sure if that's too much safeguarding though.

Any thoughts @mmenestr @kelseamann ?

edit: it should be this modal if I'm correct

Also to check - when removing a repository what would be the desired behaviour? Currently when in the Selected toggle the checkbox gets unchecked, but the repository stays there until the toggle gets toggled to All and retoggled to Selected again.

After the changes the repository would disappear from the Selected list as soon as it gets unchecked. I'm aware we're not currently consistent with the behaviour between repos / packages steps, so this might be an issue for a separate PR.

[mmenestr]

I feel like we should keep the modal... this is probably a rare scenario anyway. Also, I think that when the user unselects the repo, the packages should automatically get removed from the "selected" toggle.

In general - my guidance when it comes to unselected items is this:

If the user is in the "selected" toggle, and unchecks something, then the items should remain in the "selected" list, unchecked, until the user toggles back to "All".

However, if the user is NOT on the selected toggle, and their actions impact the selected toggle, then the items should automatically go away from the selected toggle.

Does that make sense? It basically just depends on whether the user is in the selected toggle while things are unselected or not.

[regexowl]

Also, I think that when the user unselects the repo, the packages should automatically get removed from the "selected" toggle.

Agree, alas that's currently not possible as the packages aren't linked to their origin repository.

[Andrewgdewar]

Thank you for working on the refactor! The code looks much cleaner ❤️

Looking at this from the user point of view there seems to be considerably more loading as more actions trigger a new API call including search and removing repositories.

This is linked to the question of how many repositories should we initially fetch - previously we fetched all the repositories right from the start which looked a bit more seamless when going through the step. What I mean is this:

• previously the fetching happened at the initialization of the step, but then there was no further loading / skeletons:
  

      [
        
      
          ![repos-old](https://private-user-images.githubusercontent.com/49452678/332746850-ba2204ef-bd11-4a94-8176-41256fc44b16.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTY5MDc2NzIsIm5iZiI6MTcxNjkwNzM3MiwicGF0aCI6Ii80OTQ1MjY3OC8zMzI3NDY4NTAtYmEyMjA0ZWYtYmQxMS00YTk0LTgxNzYtNDEyNTZmYzQ0YjE2LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA1MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNTI4VDE0NDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTMxZTEwZjY0MWUxMjExYWI5NmI0YTIzMjhjNTQzN2MzZTZjMWJiZDZkYzlkMWFhNWI2NDk5Mzc0NmM4YWY2NjgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.e63EtPWhTi3Jhux8d5HD5iGbfUWZnKiOVGL0R8vbz84)
        ](https://private-user-images.githubusercontent.com/49452678/332746850-ba2204ef-bd11-4a94-8176-41256fc44b16.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTY5MDc2NzIsIm5iZiI6MTcxNjkwNzM3MiwicGF0aCI6Ii80OTQ1MjY3OC8zMzI3NDY4NTAtYmEyMjA0ZWYtYmQxMS00YTk0LTgxNzYtNDEyNTZmYzQ0YjE2LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA1MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNTI4VDE0NDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTMxZTEwZjY0MWUxMjExYWI5NmI0YTIzMjhjNTQzN2MzZTZjMWJiZDZkYzlkMWFhNWI2NDk5Mzc0NmM4YWY2NjgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.e63EtPWhTi3Jhux8d5HD5iGbfUWZnKiOVGL0R8vbz84)
      
      
        
          
            
          
          
            
            
          
        
        [
          
            
          
        ](https://private-user-images.githubusercontent.com/49452678/332746850-ba2204ef-bd11-4a94-8176-41256fc44b16.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTY5MDc2NzIsIm5iZiI6MTcxNjkwNzM3MiwicGF0aCI6Ii80OTQ1MjY3OC8zMzI3NDY4NTAtYmEyMjA0ZWYtYmQxMS00YTk0LTgxNzYtNDEyNTZmYzQ0YjE2LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA1MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNTI4VDE0NDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTMxZTEwZjY0MWUxMjExYWI5NmI0YTIzMjhjNTQzN2MzZTZjMWJiZDZkYzlkMWFhNWI2NDk5Mzc0NmM4YWY2NjgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.e63EtPWhTi3Jhux8d5HD5iGbfUWZnKiOVGL0R8vbz84)
  

• now the loading state gets triggered on toggling between all/selected, searching, clearing search input, changing pagination perPage limit and removing a repo
  

      [
        
      
          ![repos-new](https://private-user-images.githubusercontent.com/49452678/332747393-f7d1785b-4bd1-4982-b8ee-cd0728780342.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTY5MDc2NzIsIm5iZiI6MTcxNjkwNzM3MiwicGF0aCI6Ii80OTQ1MjY3OC8zMzI3NDczOTMtZjdkMTc4NWItNGJkMS00OTgyLWI4ZWUtY2QwNzI4NzgwMzQyLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA1MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNTI4VDE0NDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTdjOTRjYmRlNzg2ZjkyODIyYzUyMGI0N2I4NTQ4MzBlZjllM2U4MjQ1MTBmZTA1NTdlZDYwMDMzNTAzNTFjYzkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.W7LKIywXBmeN6-6dKiTmi7Pjde6SK11066WQ4VVCucw)
        ](https://private-user-images.githubusercontent.com/49452678/332747393-f7d1785b-4bd1-4982-b8ee-cd0728780342.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTY5MDc2NzIsIm5iZiI6MTcxNjkwNzM3MiwicGF0aCI6Ii80OTQ1MjY3OC8zMzI3NDczOTMtZjdkMTc4NWItNGJkMS00OTgyLWI4ZWUtY2QwNzI4NzgwMzQyLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA1MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNTI4VDE0NDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTdjOTRjYmRlNzg2ZjkyODIyYzUyMGI0N2I4NTQ4MzBlZjllM2U4MjQ1MTBmZTA1NTdlZDYwMDMzNTAzNTFjYzkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.W7LKIywXBmeN6-6dKiTmi7Pjde6SK11066WQ4VVCucw)
      
      
        
          
            
          
          
            
            
          
        
        [
          
            
          
        ](https://private-user-images.githubusercontent.com/49452678/332747393-f7d1785b-4bd1-4982-b8ee-cd0728780342.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTY5MDc2NzIsIm5iZiI6MTcxNjkwNzM3MiwicGF0aCI6Ii80OTQ1MjY3OC8zMzI3NDczOTMtZjdkMTc4NWItNGJkMS00OTgyLWI4ZWUtY2QwNzI4NzgwMzQyLmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA1MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNTI4VDE0NDI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTdjOTRjYmRlNzg2ZjkyODIyYzUyMGI0N2I4NTQ4MzBlZjllM2U4MjQ1MTBmZTA1NTdlZDYwMDMzNTAzNTFjYzkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.W7LKIywXBmeN6-6dKiTmi7Pjde6SK11066WQ4VVCucw)
  

Which might not be necessarily bad, but it might be good to discuss which path to take from this point. What would be the advantages of handling the data via a series of API calls?

Hello! @regexowl 👋

First thanks for the extensive review.

The primary reason I am using the API in this way is consistency across our apps.
As it was the API was built around this functionality.

I understand the idea of liking to front-load all data, but in general, it is largely frowned upon as an architectural choice unless there is an inherit slowness with the API.

The usual reasons this is sited as not a good pattern in general is you can get to a point where with enough data it becomes a problem, there are also different data limits across browsers and devices, that can lock up/slow-down the web page dramatically (likely not an issue but I digress).

On content-sources, we have a front-end cache of sorts that handles the API so the loading state is not really perceived so obviously from the users point of view when looking back at previous data.

If there is some way to implement something similar with the redux toolkit that may alleviate some of the perceived slowness, but I would suggest using the API as intended.

[Andrewgdewar]

As for the feedback to removing the modal - I remember adding it for some reason, but can't really remember what the reason was 😅

I guess it was there to make double sure that the user knows they can break their image with the change? Not sure if that's too much safeguarding though.

Any thoughts @mmenestr @kelseamann ?

edit: it should be this modal if I'm correct

I spoke to @jlsherrill regarding this (SME for content) and he had basically said that the warning should be enough.
But if you would like this to be re-added I can do!

Also to check - when removing a repository what would be the desired behaviour? Currently when in the Selected toggle the checkbox gets unchecked, but the repository stays there until the toggle gets toggled to All and retoggled to Selected again.

After the changes the repository would disappear from the Selected list as soon as it gets unchecked. I'm aware we're not currently consistent with the behaviour between repos / packages steps, so this might be an issue for a separate PR.

Oh hmm, I see what you mean.
That is the exact behaviour we have today in content templates when adding and removing a repository for reference.
I may be able to add a debounce on the selected items to alleviate that extra loading.

But with the addition of the remove modal, it may be less obvious as the user will be slowed down further.

[regexowl]

The primary reason I am using the API in this way is consistency across our apps. As it was the API was built around this functionality.

Got it, sounds great!

I understand the idea of liking to front-load all data, but in general, it is largely frowned upon as an architectural choice unless there is an inherit slowness with the API.

The usual reasons this is sited as not a good pattern in general is you can get to a point where with enough data it becomes a problem, there are also different data limits across browsers and devices, that can lock up/slow-down the web page dramatically (likely not an issue but I digress).

Fair enough! I don't know how many repositories users usually have, but I admit that relying on the idea that "there can't be that many" is not a good approach. 😅

We can try to figure out if there's a way to cache some data so the loading is less obvious, but I'd say that's out of the scope of this PR for now.

Thanks a lot for the explanation!

[regexowl]

But with the addition of the remove modal, it may be less obvious as the user will be slowed down further.

Hmm, when you're looking at the debounce would it be possible to make a short screencast with and without the modal?

I think there are two open questions regarding the modal:

• how to make it obvious to the user that removing previously used repositories is a really bad idea
• and in case the way is to display the modal, when should it be displayed?

Since we probably want the newly unchecked repos to stay in Selected for a bit, it may indeed clash with the modal popping out as soon as the repo gets unselected.

Maybe we could show the modal just once when the Next button is clicked after the changes were applied? This would probably need to be handled via wizard footer then... 🤔

Or maybe we could display the alert only when a previously used repo was unselected so it pops out more?

What do you think @mmenestr?

[regexowl]

Clicked around a bunch - this looks and works great, thank you! Current way of fetching/refreshing feels much better to me! The skeletons were a bit overwhelming 💀

I'd say let's double check the module_hotfixes test, decide on the modal and debounce and then it's good to get merged.

We'll need to update msw fixtures and handlers a bit if we want to keep making use of the in-browser mocking, but that's a separate problem. Opened an issue for that: #2107

[regexowl]

Oh, one thing - could we make the search case insensitive?

[Andrewgdewar]

Oh, one thing - could we make the search case insensitive?

Good sanity check. 🎉
Added a ticket for the API here.

[ezr-ondrej]

/retest

[Andrewgdewar]

Added the modal, made it so I only warn if removing an item that was present during the initial state.
I think a debounce is no longer needed here, it feels fine and with the modal pop-up it's a non-issue.
Also changed the wording a bit as with the warning in the background, it felt a bit redundant.

I also fixed some loading/state issues with the packages tab (likely my own bugs.. 🥲 ).

[regexowl]

/retest

[regexowl]

/retest

[regexowl]

Found one bug and a tiny nitpick, but otherwise looks great to me!

[Andrewgdewar]

Okay, so I fixed the bug above, and looked into cacheing here.
I then rewrote a few things and the cache/refetch functions now work a LOT better.
I think the snappiness you are after is largely back! 🎉

[Andrewgdewar]

I have added the "search" field to the repositories mock function to get around the test failures.

[Andrewgdewar]

Fixed lint and flattened merge history.

[regexowl]

/retest

[regexowl]

/retest

[regexowl]

Looks awesome and works like magic! 🧙 Thanks for all your work on this!!