HMS-9590: refactor create and edit template worflow#763
Conversation
fd23679 to
38391bc
Compare
c1b55d7 to
75a9b69
Compare
defineContent now reads and updates data in the new template store through its DefineContentStore. Only DefineContentStore has access to the top level context - all the downstream components are communicating through the DefineContent store. This helps make components more isolated from possible changes in the top level store. Related handlers that sets architectures and osVersions were changed and derived data functions created. Fetching lists of architectures and osVersions is followed with a memoized function which selects only allowed types. DropdownGroup component is more modulurized, extracting DropdownList and extracting only the different props between architectures and osVersions.
selectSnapshots now reads and updates state in the new template request store through its SetUpDateStore. Only related variables were changed.
redhatRepositories and otherRepositories now read and update state in the new template request store through RedhatRepositoresStore and CustomRepositoriesStore respectively. Only related variables were changed and derived data logic added in the stores. Components now reference this derived data from their stores. Old leftovers in DefineContentStore which used to set data in the old template store were removed.
Initialization of template data for editing now places data into the new store. A new function initializeEditTemplateState is created for that purpose. It needs to fetch repository data to be able to set the type of a repository (hardcoded, additional, custom), which is now done imperatively in fetchTemplateRepos, since it only happens as part of the use-case and not its main purpose, and it needs to happen only once in the beginning of the edit template workflow.
Also deleting the related temporary bridge for setting data from the old store to the new. This commit is the final step in converting the top template store to use a new structure of a template storage.
Making rules for checking enabled status of each step explicit. Each step has its function. Defining new object which groups individual steps and is easy to add to.
to better capture its purpose.
Use early return to make different versions of the component more explicit.
The use-case has been taken out of UI into a function in its own file. A hook is composing the use-case to be able to use it in UI. Also the api mutation function has been placed under the createTemplate feature folder since it is the only place where it is used. A ports file has been created to define what is the shape of data that the domain core accepts, which functionalities it provides to the UI (input ports) and what other services the core relies on (output ports), to make explicit the most important pieces.
Similarly to createTemplate, the confirmEditTemplate use-case has been taken out of UI into a function in its own file. A hook is composing the use-case to be able to use it in UI. The related mutation has been placed under the editTemplate folder to keep the related code together. Also getTemplate use-case has been extracted out into its own file. A ports file has been created to mark the most important parts.
chooseHardcodedUUIDs and initializeSystemsLists have been put into their explicit functions. chooseHardcodedUUIDs replaces multiple useEffects, and saves into TemplateStore only the result that we want to keep for template creation. For this reason declarative hook was replaced with imperative fetch and placed under the defineContent folder. initializeSystemsLists has been just taken out of DefineContentStore into its explicit function. Dropdown logic for expanding and collapsing dropdowns has been extracted into its hook. A ports file has been created to mark the most important parts.
validateDependencies use-case has been extracted and changed together with related UI changes in DependencyNotification component. The use-case used to be spread in SetUpDateStore. Now it is in one function and composed in a hook. Fetching repositories is now done imperatively, since it depends on branching. Fetching snapshots has been extracted into its file. DependencyNotification now notifies about the progress of validating dependencies after a user picks a specific snapshot date. It alerts also in case of no issues. Wording has been changed to better explain what is the problem in case of issues. Api in SetUpDateStore has been made more granular and memoized into 3 context providers. A ports file has been created to define explicit types for the most important functions.
restrictFutureDates has been put into its own file. Properties of components have been defined as objects first and decision to showDateInput has been put into a function instead of using a Hide component.
As there is only one use-case in reviewTemplateRequest, ReviewTemplateStore has been replaced with formatTemplateReview function and composed in a hook. ReviewTemplateStore was deleted. ReviewTemplateContent component has been made always expanded, to help user see and review full template request right away, since there are not that many values. A ports file has been created to define explicit types for the most important FormattedTemplateReview use-case.
Changed in UI: template name --> title, description --> detail, Updated PW tests to accomodate those text changes in UI. DetailInputFields has been split into TemplateTitle and TemplateDetail components. Logic of validating the inputs has been put into validateTitle and validateDetail use-cases and underlying validateUserInput function extracted. A ports file has been created to define explicit types for the two most important functions.
In redhatRepositories and otherRepositories both. Generalized useSortReposTable hook has been put into shared folder.
toggleSelectedRepository has been extracted into its own function and composed in a hook.
RedhatRepositories api has been divided into 5 granular context stores and data in the stores have been memoized. Related apis have been changen, also in downstream consuming components. TableControls and RedhatRepositoriesTable have been divided into constituent subcomponents. Early returns have been used in top level RedhatRepositoriesStep and in TableControls. Query that fetches and keeps redhat repositories have been extracted and placed under api, together with the related useGetRedhatRepositories hook that takes care of triggering the query. A ports file has been created to define explicit types for the most important functions.
toggleOtherRepository has been extracted into its own function and composed in a hook.
refreshRepositories has been extracted into its own function and composed in a hook. Related consuming RefreshListButton component has been extracted from TableHeading component.
CustomRepositories api has been divided into 5 granular context stores and data in the stores have been memoized. Related apis have been changen, also in downstream consuming components. TableControls and CustomRepositoriesTable have been divided into constituent subcomponents. Early returns have been used in top level CustomRepositoriesStep and TableControls. Query that fetches and keeps other repositories have been extracted and placed under api, together with the related useGetOtherRepositories hook that takes care of triggering the query. A ports file has been created to define explicit types for the most important functions. --- This commit concludes create and edit template workflow refactor.
There was a problem hiding this comment.
Sorry @Starle21, your pull request is larger than the review limit of 150000 diff characters
|
Thanks so much for putting this PR together! Since there’s a lot to digest here, I’m treating this first pass as a high-level look at the concepts rather than a line-by-line review. I really appreciate the initiative you took here. There are so many fantastic improvements! I just have a few thoughts on structural concepts that might be good to align on as a team before we apply them codebase-wide. ✨ What I really like about these changes
🗣️ What we should discuss with the wider team
Let me know what you think about these points! |
| export type LatestSnapshot = true; | ||
| export type UseLatestSnapshot = LatestSnapshot | WithoutLatestSnapshot; | ||
|
|
||
| // Title and Detail |
There was a problem hiding this comment.
i'm curious about the benefit of the type aliases. for example, could the FilledTemplateTitle and EmptyTemplateTitle types be simplified to TemplateTitle = string (or TemplateTitle = string | '')? i notice this pattern is used in a few places, and simplifying these could reduce the number of mental steps, but maybe there is a use case i'm missing :)
| import { useValidateTitle } from '../../core/use-cases/validateTitle'; | ||
|
|
||
| export const TemplateTitle = () => { | ||
| const { validateTitle, isValidated, error, text } = useValidateTitle(); |
There was a problem hiding this comment.
using a relatively simple workflow (validating the template title) as an example, it looks like this structure would require understanding concepts and files in 5 different layers (UI, use-case, domain, ports, store). i see the benefits and like that all related code is grouped together! i also think that this may add to the initial learning curve for more complex workflows.
maybe we could have examples or documentation showing when to use which layer? i think the steps to building a feature slice part of your notes and a simple example could be helpful to keep around. i also like the glossary of terms you've added, this could be beneficial to keep too
There was a problem hiding this comment.
agree! 👍🏼
I think it would be nice to keep something like this somewhere (markdown file at the root of features/ 💭) and an example with code snippets could be nice 😅
_and while doing so, it might be nice to make it simpler 📝 (more grug brain friendly) 😆
|
|
||
| ### Introducing 2 conceptually different structures | ||
|
|
||
| The new code introduces a different structure from the old one. It might be problematic to maintain the 2 types of structures. We might want to gradually convert rest of the code. This also might be a problem though if we as a team were not conviced it would pay off. |
There was a problem hiding this comment.
i share your concerns here, especially regarding maintaining 2 different structures in the code (if we don't plan to refactor everything and will need to maintain both indefinitely). i am worried that my lack of experience with the new structure could hinder progress by taking longer to implement things or completely messing up what you've proposed, and maintaining both structures may add to that 😮💨
i think it would be good to target one structure and migrate everything incrementally. we may not need to use the same model for every folder layout, but having one high-level structure in the code (the one you proposed here) might help with navigation and future contributions
|
thank you for all your work on this! i really like these improvements in particular:
i also like Kate's idea of looking at Patternfly's structure and seeing where we could align with theirs! these things are generally what i'm cautious or have questions about:
i also agree with the points you've laid out in your notes about what to watch out for, and added a few initial questions around the above concerns pointing to some examples. overall i think this refactor addresses a lot of real problems in the code and i really appreciate all the effort you've put into making it better 🥹 i'm curious how we can find ways to reduce the learning curve and make it easier for the team to adopt |
There was a problem hiding this comment.
Thanks for organising the types! I've been looking into them today to give you some more feedback
I believe we're currently using more types than necessary and eliminating some of them could make the code easier to read and maintain
For example, we could replace these with their literal values directly: WithoutSnapshotDate and LatestSnapshot. HardcodedUUID, AdditionalUUID, OtherUUID - all just string
Also, we could make more use of discriminated unions:
type TemplateRequestFinalizedWithDate = {
snapshotDate: SnapshotDate;
isLatestSnapshot: WithoutLatestSnapshot;
// ...
};
type TemplateRequestFinalizedWithLatestSnapshot = {
snapshotDate: WithoutSnapshotDate;
isLatestSnapshot: LatestSnapshot;
// ...
};
type TemplateRequestFinalizedBase = {
selectedArchitecture: AllowedArchitecture;
selectedOSVersion: AllowedOSVersion;
hardcodedUUIDs: string[];
additionalUUIDs: string[];
otherUUIDs: string[];
title: string;
detail: string;
};
export type TemplateRequestFinalized =
| (TemplateRequestFinalizedBase & {
snapshotDate: string;
isLatestSnapshot: false;
})
| (TemplateRequestFinalizedBase & {
snapshotDate: '';
isLatestSnapshot: true;
});
This could help us reduce the total number of type definitions and make it clearer what the actual runtime values are. What do you think?
There was a problem hiding this comment.
fully agree here 👍🏼
also, this being types.compound.ts is a bit strange, it seems like just template partial types 💭
There was a problem hiding this comment.
first off: wow! 😮 that's a lot of work! thank you!! 👏🏼😌
(the commit splitting, makes this sort of reviewable and shows of the steps, thx! although still not line by line reviewable so we will have to trust our tests and manual testing 😄)
never seen or worked with a frontend code written like this, so there is a lot to learn and get used to 🤓
the vertical/feature slice approach seems quite cool! time will tell how nice it is to work in, but in general it looks good, and I have high hopes! let's try it out! ✨
I am not the first to review this and have to agree with Kate and Bryttanie on most of the stuff they pointed out 💯
- all the amazing stuff 🤩: decoupling/grouping, UX fixes like the warning, extracting logic out of components, early returns, possibly better maintainability in the future
- and the worrying 😨: complexity (context hell, amount of abstractions, learning curve), maintenance of both structures (<- this might be my biggest concern), too many folders (and new names 😆)
you know all the pros and why this is nice, that's why you did this 😄
so here are a few comments on the cons from someone unfamiliar (and maybe some suggestions how to make this more digestible and easier to slot into the project as a whole?)
(not all of these are necessary change requests, just my opinions, some of which might be caused by my inexperience/unfamiliarity) 💭:
- contexts being called
SomethingApiorSomething[Derived]Statecan be a bit confusing, I still don't really get the difference - too many folders and no "entry point"
- this made me a bit lost initially, when you open the feature, workflow or step directories you only get folders, and the steps have
coreandui, this prompts a question where should I start looking first? 😵💫 - I think "exploding" the UI folder would help, i.e. the
SomethingStep[.test].tsxfiles would be at the root of the step folder and there wouldn't be anuifolder, just acomponentsfolder 💭 this would help with navigating the code imho.
(and also maybe pulling the insides ofuifromworkflowinto the root ofcreateAndEditTemplate?) - and/or "exploding" the core folders (so that ports and types are at the root) and renaming domain to helpers (I think you even mentioned doing that rename)
- this made me a bit lost initially, when you open the feature, workflow or step directories you only get folders, and the steps have
- rename
FirstEmpty-->Option|Optional, that might be clearer/more explicit - really a nitpick: rename
DependencyNotficationand subsequent hooks, etc. to something likeDependency[Status|Warning|Banner]? we mostly use the name notification for toast notifications, so this could be clearer what's what 😅 - I would be cautious with the amount of stores, there is quite a lot of them (ex.: repo steps), which creates some complexity imo 😵💫
- as Kate mentioned: "bit of light coupling can be helpful for readability and DX ... just so we don't inadvertently increase cognitive load"
which I think might be the case here (or getting really close to) so that we don't have a context hell (and possibly in other places)
- as Kate mentioned: "bit of light coupling can be helpful for readability and DX ... just so we don't inadvertently increase cognitive load"
some more on-the-go thoughts, while reviewing commit by commit, in other comments 💭
great work! 💪🏼👏🏼
There was a problem hiding this comment.
fully agree here 👍🏼
also, this being types.compound.ts is a bit strange, it seems like just template partial types 💭
| // ----------------------------- | ||
| // template returned from server | ||
|
|
||
| export type FullTemplate = { |
There was a problem hiding this comment.
why the name Full? I find that a bit weird 🤔
that's a Template, I would strip out the Full, same goes for repositories and snapshots 😅
There was a problem hiding this comment.
just my opinion, not a change request:
I kinda dislike redefining the "globally" used types here 🫤
helper types or stuff that's gonna be used only in the feature (specific template wizard types), sure, go wild! 😄
but when you redefine a template here it's going to contribute to the difficulty of doing work across the two different "codebases" that are going to be in this repo IMHO 💭 (and also updating a type will have to be done across different places) 🤔
I know that would break the isolation of the feature and vertical slicing concept a bit, but this will have to slot into an already existing codebase (which might be the biggest hurdle as it's not a new project) ⚖️
not sure what would be the best approach here, I don't have experience with stuff like this, this is just my opinion 😅
| } from 'features/createAndEditTemplate/shared/types/types.compound'; | ||
| import { formatTemplateDate } from 'features/createAndEditTemplate/shared/core/formatTemplateDate'; | ||
|
|
||
| type CreateTemplate = (templateRequest: TemplateRequestFinalized) => TemplateRequestToSend; |
There was a problem hiding this comment.
what's the benefit of type aliasing the function signatures like this? 💭 (maybe if you were passing this somewhere, but that's not case afaik)
I find it quite hard to read (haven't seen this much prior to this review), maybe just in-lining these would be cleaner? 🧼 🤔
There was a problem hiding this comment.
I would use a different name here 😵💫
"systems" is a little miss-leading since that's something usually connected to Patch and actual systems, while this just gets the available template content config (repository parameters) options 💭
| import { useValidateTitle } from '../../core/use-cases/validateTitle'; | ||
|
|
||
| export const TemplateTitle = () => { | ||
| const { validateTitle, isValidated, error, text } = useValidateTitle(); |
There was a problem hiding this comment.
agree! 👍🏼
I think it would be nice to keep something like this somewhere (markdown file at the root of features/ 💭) and an example with code snippets could be nice 😅
_and while doing so, it might be nice to make it simpler 📝 (more grug brain friendly) 😆
Summary - CREATE AND EDIT TEMPLATE WORKFLOW REFACTOR
The goal is to improve readability, modularity and changeability of the create and edit template workflow. The overall aim is to make functionality explicit and easy to change. This can be achieved by making each step independent on the others. Each step then encapsulates domain logic (core, use-cases), api requests and UI.
Problems
Files in the Pages folder are a ball of mud. They are an incoherent mesh of code. They try to combine multiple use-cases, their data transformations and derived data for a pile of UI components each of them gathers. They are written as one giant component, easily reaching 500 lines of code. There is a page (ContentListTable), which reaches 930 lines. AddOrEditTemplate folder, where the create and edit template workflow resides, is no exception.
Not only it is bad from a performance perspective - when there is any state change, a whole giant component has to rerender, it is hard to understand where one functionality ends and another starts. It is hard to understand what a particular piece of code does. Which use-case does it belong to? There is no
separation of concerns. Each time we want to change a detail on a page of any step, we have to understand and go through all of it. We have to mentaly group together the parts that go together. It would be better to make this explicit, to define separate functions and components for these groups once so we don't have to keep doing this again and again every time we work with a particular Page.Remedies
In general, there are 2 concepts used to fix the ball of mud -
separation of concernsandput together what changes together (coherence).In UI, big components are split into constituent subcomponents. These subcomponents read the logic that concerns only them from a context store that each step has. Early returns help one see explicitly what versions a component can be in, depending on a state.
Logic is taken out of UI and placed into a particular context store of each step. Components only read and update data through their store api. If there was a change in logic in future, it would only have to change in a store, the api would stay the same and no change would have to be done in consuming components.
Business processes are translated in software as use-cases. They are the main reason any business application exists. Multiple functions, spread over multiple useEffects and setStates (as in DefineContentStep or SetUpDateStep) has been used up until now. Each use-case is now encapsulated in one function (chooseHardcodedUUIDs, validateDependencies). It groups multiple steps in a data transfomation pipeline, where only the ending state is the one that is saved.
Hooks are used as a composition tool, so use-case functions can stand alone, but at the same time communicate with the rest of the workflow, read and set state in the top level TemplateStore context.
To make the use-cases and domain entities explicit, extensive typing is employed. Types are defined using type aliases. Types define domain, both data in its various states (TemplateRequestStartStep, TemplateRequestFurtherSteps, TemplateRequestFinalized) and data transformations (use-cases: CreateNewTemplate, ToggleSelectedOtherRepository). Compound types are defined by ANDing or ORing simple types (so called algebraic types).
Ports files make explicit the most important functions on the "edges" of a domain core. Input ports define the shape of data that a domain core accepts and which functionalities it provides to UI. Output ports declare what other services the core relies on, without which it cannot function.
Related code is grouped into one folder (feature) - application core with domain types, domain functions and use-cases; client store; api and UI. It is a so-called vertical feature slice, that cuts through multiple technical layers (api, runtime store, domain logic), that are usually always touched when we need to make a change. It is collocated together, to improve coherence of the workflow.
When multiple steps use the same function, it is extracted into the shared folder. It is better to make sure components from multiple steps don't import functions directly from one another, which would start to create a ball of mud again. Workflow folder with TemplateStore can be considered as being "on top" of other steps that communicate with it through context stores or in use-case composition hooks. Otherwise, workflow only composes the overall create or edit workflow steps. EditTemplate feature is a similar to workflow, that it is on top of other steps and they read directly from it.
If a whole application is structured with vertical feature slices, outside of them there is only a generic and reusable code left. We can define reusable hooks, UI components without any business logic (presentational components), api service, generic react context runtime store or persistence service. If an api or UI code is particularly coupled with a feature logic, it is put under the feature slice and not in generic services.
Commits and phases in the PR
To help with reviewing the PR, there can be 4 phases distinguished in the commits:
move: AddOrEditTemplate under featuresrename: AddOrEditTemplate -> TemplateModalBaseextract: queryClient from AddTemplateContextrename: AddTemplateContextProvider -> TemplateStoreextract, change: structure of CustomRepositoriesStorePlease open and see detailed notes for the refactor in
src/features/notes-refactor.If you feel like you want to learn more about the concepts, you can check out documentation in the
src/features/notes-refactorfolder. However, it is not at all necessary and I will remove it before merging.Testing steps
Manual testing
Run either against stage or against locally running backend. Try out both create and edit workflows from start to end to see that either a new template is created or a change to an already existing template is made.
Playwright tests
For testing with playwright locally, you need to run the backend PR below.
make repos-minimalcommand imports extra repositories that the template workflow need to run correctly:#testwith content-services/content-sources-backend#1358