Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: Reset state for workflow item history when changing tabs #318

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

mathiashoeld
Copy link
Contributor

Changes

When a user views the workflow item details and clicks on history and
then back to another tab, the history state is now reset, so the
calculation of offset and limit are correct.

Add e2e test that makes sure this does not happen again.

Refactor e2e tests, so that 'data-test' is used instead of 'id'.

Related issues

Closes #317

How to test

When viewing a workflow item's history, there should be no issues.

@mathiashoeld mathiashoeld requested a review from kevinbader June 6, 2019 10:17
@mathiashoeld mathiashoeld force-pushed the 317-fix-crashing-history branch 2 times, most recently from 224e03f to 1fb6c4f Compare June 6, 2019 11:32
Copy link
Collaborator

@gonzochic gonzochic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe to folder structure can be improved. I would move WorkflowItemDetails into the WorklowItem folder

@@ -35,7 +35,8 @@ export default function reducer(state = initialState, action) {
currentHistoryPage: action.currentHistoryPage,
isHistoryLoading: false
});

case CLOSE_WORKFLOWITEM_HISTORY:
return initialState;
case CLOSE_WORKFLOWITEM_DETAILS:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use fallthrough and handle logout action

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also added the LOGOUT action here

@@ -144,6 +145,12 @@ function WorkflowDetails({
},
[showWorkflowDetails]
);
useEffect(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effect is called every time, even if it is not necessary. I believe this hook has to move into the component itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed implementation of <WorkflowitemHistoryTab> to class based and used componentWillUnmount to reset state

@mathiashoeld mathiashoeld force-pushed the 317-fix-crashing-history branch from 2fa66be to af0daf7 Compare June 11, 2019 11:54
import { fetchNextWorkflowitemHistoryPage, resetWorkflowitemHistory } from "./actions";

class WorkflowitemHistoryTab extends React.Component {
constructor(props) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove, not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

When a user views the workflow item details and clicks on history and
then back to another tab, the history state is now reset, so the
calculation of offset and limit are correct.

Add e2e test that makes sure this does not happen again.

Refactor e2e tests, so that 'data-test' is used instead of 'id'.

Closes #317

Rework based on review

- Moved WorkflowitemHistoryTab into Workflows
- Rewrote WorkflowitemHistory to class based component
- Removed effect hook for resetting history state and moved it to
ComponentWillUnmount
- Fixed e2e-tests
- Added LOGOUT action to WorkflowitemHistory reducer
@mathiashoeld mathiashoeld force-pushed the 317-fix-crashing-history branch 2 times, most recently from 73a7d79 to d7ef6e5 Compare June 11, 2019 12:51
@mathiashoeld mathiashoeld merged commit 780cfa1 into master Jun 11, 2019
@mathiashoeld mathiashoeld deleted the 317-fix-crashing-history branch June 17, 2019 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: Workflowitem history crashes
2 participants