refactor the remaining service modules which contained internal state into stores/views #2675
Conversation
| message: response.ok ? "success" : `error:${responseStatusText(response)}`, | ||
| }; | ||
| } catch (err) { | ||
| console.log(err); |
There was a problem hiding this comment.
it was log where this function existed before... are you suggesting it should be .error? It still shows as an error in the console due to logging an exception
There was a problem hiding this comment.
it would be more correct, also we do that in other places
| const [, data] = await serviceControlStore.fetchTypedFromServiceControl<EmailNotifications>("notifications/email"); | ||
| result = data; | ||
| } catch (err) { | ||
| console.log(err); |
| async function saveDeletedRedirect() { | ||
| const result = handleResponse(await deleteFromServiceControl(`redirects/${selectedRedirect.value.message_redirect_id}`)); | ||
| const result = handleResponse(await serviceControlStore.deleteFromServiceControl(`redirects/${selectedRedirect.value.message_redirect_id}`)); |
There was a problem hiding this comment.
we have a redirectsStore, I think the "delete" should be part of it
| showEdit.value = false; | ||
| const result = handleResponse( | ||
| await postToServiceControl("redirects", { | ||
| await serviceControlStore.postToServiceControl("redirects", { |
There was a problem hiding this comment.
we have a redirectsStore, I think the "post" should be part of it
| async function loadEndpoints() { | ||
| const [, data] = await useTypedFetchFromServiceControl<QueueAddress[]>("errors/queues/addresses"); | ||
| const [, data] = await serviceControlStore.fetchTypedFromServiceControl<QueueAddress[]>("errors/queues/addresses"); |
There was a problem hiding this comment.
should this method calls be part of the errorsStore?
| return { | ||
| configuration, | ||
| loadConfig, | ||
| refresh, |
There was a problem hiding this comment.
Do we need to expose refresh given that we have the watch and serviceControlUrl is the only state that would trigger a change?
| async function removeEndpoint(endpointName: string, instance: ExtendedEndpointInstance) { | ||
| try { | ||
| await useDeleteFromMonitoring("monitored-instance/" + endpointName + "/" + instance.id); | ||
| await serviceControlStore.deleteFromMonitoring("monitored-instance/" + endpointName + "/" + instance.id); |
There was a problem hiding this comment.
this should be moved to the monitoringStore
| async function getIsRemovingEndpointEnabled() { | ||
| try { | ||
| const response = await useOptionsFromMonitoring(); | ||
| const response = await serviceControlStore.optionsFromMonitoring(); |
There was a problem hiding this comment.
this should be moved to the monitoringStore
|
|
||
| setActivePinia(createTestingPinia({ stubActions: false })); | ||
| const serviceControlStore = useServiceControlStore(); | ||
| serviceControlStore.refresh(); |
There was a problem hiding this comment.
This is just an observation.
There is a bit of repetition in tests always setting this up, we may need to come up with a "base" to do all this eventually.
setActivePinia(createTestingPinia({ stubActions: false }));
const serviceControlStore = useServiceControlStore();
serviceControlStore.refresh();
|
|
||
| // eslint-disable-next-line promise/catch-or-return,promise/prefer-await-to-then,promise/valid-params | ||
| Promise.all([editRetryStore.loadConfig(), configStore.loadConfig()]).then(); | ||
| Promise.all([editRetryStore.loadConfig(), configStore.refresh()]).then(); |
There was a problem hiding this comment.
with the refactor done in configStore, I am unsure we need to call the refresh here?
|
|
||
| // eslint-disable-next-line promise/catch-or-return,promise/prefer-await-to-then,promise/valid-params | ||
| Promise.all([editRetryStore.loadConfig(), configStore.loadConfig()]).then(); | ||
| Promise.all([editRetryStore.loadConfig(), configStore.refresh()]).then(); |
There was a problem hiding this comment.
it seems that editRetryStore is only used by this store, which begs the question whether we need a sub store at all?
src/Frontend/src/views/throughputreport/setup/ConnectionSetupView.vue
Outdated
Show resolved
Hide resolved
| emailToggleSuccessful.value = true; | ||
| } else { | ||
| emailToggleSuccessful.value = false; | ||
| console.log(result.message); |
There was a problem hiding this comment.
not really; it just seemed strange to me that there's code to craft these meaningful failure messages and those messages are never used
There was a problem hiding this comment.
maybe raise a bug/improvement about it?
| { immediate: true } | ||
| ); | ||
| const endpointColor = hexToCSSFilter("#929E9E").filter; | ||
| const endpointColor = hexToCSSFilter("#777F7F").filter; |
There was a problem hiding this comment.
at some stage this colour got out of sync with the other icons
There was a problem hiding this comment.
is it possible to address that? using a variable maybe?
There was a problem hiding this comment.
I have just done a full search on the code base, and there are usages of hexToCSSFilter("#929E9E"), so why are you saying this code got out of sync?
There was a problem hiding this comment.
i'll check and fix the remaining filter references. We can't use CSS variables here, since it's done in code rather than styles, so if other styles change then the author needs to remember to change the relevant filters.
It is possible to achieve what we're doing here with styles, but at the moment Safari doesn't render it correctly
| // if Audit connection test fails, we will return true. | ||
| // the connection test will return true if there are no Audit instances configured. | ||
| if (!testResults.value?.audit_connection_result.connection_successful) { | ||
| //TODO: should this be a warning rather than an error? |
There was a problem hiding this comment.
we have no concept of warning in this scenario
There was a problem hiding this comment.
we can create one though... we have the warning triangle used elsewhere in the app
There was a problem hiding this comment.
we can raise an issue about it
… ServiceControlAvailable
2 participants
part of #1905