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

Refresh aggregate infos after command execution #7

Merged
merged 2 commits into from
May 1, 2020

Conversation

codeliner
Copy link
Contributor

This PR adds a quick fix for auto refreshing aggregate list and aggregate details after command execution. It is not perfect, because it is purely based on command dialog being closed.

A better way would be to refresh after successful command execution. This way we could automatically close the dialog and trigger refresh. In case command execution fails the dialog would stay open and no refresh is triggered. @martin-schilling this would be perfect case for an event dispatcher redux middleware (you remember our discussion about the topic?).

Anyway, I think the quick fix is ok for now. It definitely increases UX because at the moment you have to refresh the browser to see updates.

Copy link
Collaborator

@martin-schilling martin-schilling left a comment

Choose a reason for hiding this comment

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

Looks good and should to the job for now. I think, however, that I am going refactor it to keep this logic inside the sagas later to prevent unnecessary state from being passed around. Or was there a special reason you did it this way that I am not seeing?

@martin-schilling
Copy link
Collaborator

Oh seems like I missed your comment. Using an event dispatcher middleware, like we discussed, would work but you want to reload state anyway, right? Isn't that something that can simply be triggered through the redux saga handling the command exectution? https://github.com/event-engine/cockpit/blob/master/src/saga/executeCommandFlow.ts#L17

@codeliner
Copy link
Contributor Author

Oh wait you're right! I was a bit too fast with my quick fix :D I'll check the saga option. Missed the fact that queries are dispatched from effect functions but state is selected from redux store. So dispatching same queries from a saga should refresh the components 👍

@codeliner
Copy link
Contributor Author

Ok, let's do the refactoring later in the follow up #8
At the moment the saga does not have enough information. It only knows about the command, but not which aggregate data should be fetched again.

@codeliner codeliner merged commit 6ef3eef into master May 1, 2020
@codeliner codeliner deleted the feature/refresh_after_cmd_exec branch May 1, 2020 19:49
@martin-schilling
Copy link
Collaborator

martin-schilling commented May 1, 2020

Ok, let's do the refactoring later in the follow up

I already collected some other refactoring tasks for later. Maybe opening up a board to preserve those things would be a good idea?

It only knows about the command, but not which aggregate data should be fetched again.

Yes I thought about that too. I think it would probably be a good idea to trigger some action here https://github.com/event-engine/cockpit/blob/master/src/AggregateDetailsPage.tsx#L15-L17 to push this information into the redux store and use it as the source of truth. Then any sub-components would simply rely on that information instead of getting it from the parent through the props. The required information would then be available for the sagas too.
Do you have a better idea?

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.

2 participants