-
Notifications
You must be signed in to change notification settings - Fork 26
implement MessagesStore to perform servicecontrol API calls #2712
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
Conversation
…e and fix autorefresh interval change
# Conflicts: # src/Frontend/src/stores/MessageStore.ts
we used to have 3 views with almost identical functionality, except the API call was made with different parameters. The aborting of calls is so that the logic can be shared in a single store, but if the user switches to another view utilising the same store while an API call is in process, the return of the API call doesn't result in the list data being set to the wrong list |
# Conflicts: # src/Frontend/src/components/audit/AuditList.vue # src/Frontend/src/components/failedmessages/DeletedMessageGroups.vue # src/Frontend/src/composables/autoRefresh.ts # src/Frontend/src/stores/DeletedMessageGroupsStore.ts
…dmessages and fix merge atavisms
# Conflicts: # src/Frontend/src/components/failedmessages/DeletedMessages.vue
| const { count: requiresFullFailureDetailsSubscriberCount, inc, dec } = useCounter(0); | ||
| function requiresFullFailureDetails() { | ||
| onMounted(() => inc()); | ||
| onMounted(() => inc()); //NOTE: not forcing a refresh here since we expect the view utilising this store to also setup a refresh on mount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something that does not line up with this store. To me, it feels like we actually should have two different stores.
And adding this comment makes it more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a separate store for setting 2 extra refs? I don't think that passes the sensible test. I'd be more persuaded to remove this property and just allow the extra API calls to always be made, regardless of whether they're needed or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had a better look at ConnectionsAndStatsStore, and it seems to have a mix of concerns.
I understand the need for this store, given that we need to display message stats at the top nav, but the archivedMessageCount and pendingRetriesMessageCount do not seem to belong here. To me, these belong in the FailedMessagesStore.
If we refactor these out of here and into the FailedMessagesStore, we no longer need to have this extra complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll spike it on another branch
johnsimons
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR
| const { count: requiresFullFailureDetailsSubscriberCount, inc, dec } = useCounter(0); | ||
| function requiresFullFailureDetails() { | ||
| onMounted(() => inc()); | ||
| onMounted(() => inc()); //NOTE: not forcing a refresh here since we expect the view utilising this store to also setup a refresh on mount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had a better look at ConnectionsAndStatsStore, and it seems to have a mix of concerns.
I understand the need for this store, given that we need to display message stats at the top nav, but the archivedMessageCount and pendingRetriesMessageCount do not seem to belong here. To me, these belong in the FailedMessagesStore.
If we refactor these out of here and into the FailedMessagesStore, we no longer need to have this extra complexity.
part of #1905, extending #2675 to remove direct SC calls from views