implement revert logic by using originalSelector #603
Conversation
| @@ -18,6 +19,7 @@ const AssertionCardSelectorList: React.FC<IProps> = ({selectorList, pseudoSelect | |||
| <S.SelectorValueText>{value}</S.SelectorValueText> | |||
| </S.Selector> | |||
| ))} | |||
| <S.SelectorValueText>{originalSelector}</S.SelectorValueText> | |||
| {pseudoSelector && ( | |||
There was a problem hiding this comment.
only showing for debugging proposes
There was a problem hiding this comment.
Ok, should we remove it then?
| @@ -11,6 +11,7 @@ const AssertionResults = ({allPassed = false, results = []}: TRawAssertionResult | |||
| id: uniqueId(), | |||
| selector, | |||
| spanIds: AssertionService.getSpanIds(resultList), | |||
| originalSelector: selector, | |||
There was a problem hiding this comment.
creating the field on AsserionResult
There was a problem hiding this comment.
Do we want this in the model? I see we also have a assertionResultsToDefinitionList in web/src/redux/slices/TestDefinition.slice.ts
There was a problem hiding this comment.
I think it is good here for the moment, but I get your point
| setSelectedAssertion: CaseReducer<ITestDefinitionState, PayloadAction<string>>; | ||
| }, | ||
| 'testDefinition' | ||
| >({ |
There was a problem hiding this comment.
add better typescript typings
There was a problem hiding this comment.
I'm always up for this. Should this mean that we can drop the types from each of the reducer functions as well?
There was a problem hiding this comment.
Hmm 🤔 I'm not sure about this. IMO we should let typescript infer it when possible. It's doing it already.
| return { | ||
| ...def, | ||
| ...definition, | ||
| originalSelector: def.originalSelector, |
There was a problem hiding this comment.
need to ensure originalDefinition is carried over
There was a problem hiding this comment.
Hey @cescoferraro gj man, I left some comments and concerns. Let me know what you think
| @@ -17,8 +18,15 @@ interface TAssertionCardProps { | |||
| onEdit(assertionResult: TAssertionResultEntry): void; | |||
| } | |||
|
|
|||
| export function useRevertDefinitionCallback(originalSelector: string): () => void { | |||
There was a problem hiding this comment.
Instead of having the dispatch here, we should be able to wrap it up at the TestDefinitionProvider like the rest of the functions.
| {isDraft && ( | ||
| <> | ||
| <S.StatusTag>draft</S.StatusTag> | ||
| <Tooltip color="white" title="Revert Assertion"> |
There was a problem hiding this comment.
It is kind of weird that both the tooltip an the actual message have the same text
| @@ -18,6 +19,7 @@ const AssertionCardSelectorList: React.FC<IProps> = ({selectorList, pseudoSelect | |||
| <S.SelectorValueText>{value}</S.SelectorValueText> | |||
| </S.Selector> | |||
| ))} | |||
| <S.SelectorValueText>{originalSelector}</S.SelectorValueText> | |||
| {pseudoSelector && ( | |||
There was a problem hiding this comment.
Ok, should we remove it then?
| setSelectedAssertion: CaseReducer<ITestDefinitionState, PayloadAction<string>>; | ||
| }, | ||
| 'testDefinition' | ||
| >({ |
There was a problem hiding this comment.
I'm always up for this. Should this mean that we can drop the types from each of the reducer functions as well?
| console.log('=========='); | ||
| console.log(current(d)); | ||
| console.log(originalSelector); | ||
| if (d?.originalSelector === originalSelector) { |
There was a problem hiding this comment.
What happens when the user tries to revert a newly created assertion? in that case we should wipe it from the store isn't it?
| if (def.selector === selector) return definition; | ||
| if (def.originalSelector === selector) | ||
| return { | ||
| ...def, |
There was a problem hiding this comment.
I think we can avoid spreading the original dev, we can just do it for the new def + the original selector
cescoferraro
force-pushed
the
563-revert
branch
5 times, most recently
from
9a7e6d9
to
03ed2af
Compare
| @@ -11,6 +11,7 @@ const AssertionResults = ({allPassed = false, results = []}: TRawAssertionResult | |||
| id: uniqueId(), | |||
| selector, | |||
| spanIds: AssertionService.getSpanIds(resultList), | |||
| originalSelector: selector, | |||
There was a problem hiding this comment.
I think it is good here for the moment, but I get your point
| {payload: {definition, selector}}: PayloadAction<{definition: TTestDefinitionEntry; selector: string}> | ||
| ) { | ||
| revertDefinition(state, {payload: {originalSelector}}) { | ||
| // @ts-ignore |
| // @ts-ignore | ||
| state.definitionList = state.definitionList | ||
| .map(d => { | ||
| if (d?.originalSelector === originalSelector) { |
There was a problem hiding this comment.
Does this cover the scenario of reverting a newly created assertion?
| {isDraft && ( | ||
| <> | ||
| <S.StatusTag>draft</S.StatusTag> | ||
| <Button type="link" data-cy="assertion-action-revert" onClick={resetDefinition}> |
There was a problem hiding this comment.
I'm wondering if this onClick also needs the e.stopPropagation(); similar to what we have in the Edit/Delete icons.
This PR add revert logic to individual assertions
Changes
Checklist