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.

Sign up for GitHub

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

Port forward: ui fixes and better error handling #56

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Port forward: ui fixes and better error handling #56

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f1aef4
fix(port-forward): move destination zone combobox
kevincela Oct 11, 2023
2c2e6d1
fix(port-forward): handle firewall config error
kevincela Oct 11, 2023
9a5a702
fix(port-forward): clear errors on drawer close
kevincela Oct 11, 2023
cf575cb
fix(port-forward): use NeEmptyState for empty page
kevincela Oct 11, 2023
acc8aa1
fix(port-forward): fix error cleanup on close
kevincela Oct 11, 2023
f81f37b
fix(port-forward): refetch items when drawer is shown
kevincela Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 28 additions & 13 deletions src/components/standalone/firewall/CreateOrEditPortForwardDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,25 @@ watchEffect(() => {
})

function close() {
if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this condition needed?

Copy link
Collaborator Author

@kevincela kevincela Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not strictly needed, but if the error is given by a failed request of the form's options then clearing it could be confusing if the user reopens it again (they would see no error notification, but the options still aren't available), whereas for edit/create request errors it makes sense because the item gets cleared on exit. Maybe at this point we should simply refetch the options every time the drawer is opened, in case of an error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, if the drawer need some data we usually fetch it every time it opens, so we can get updated data. If the code doesn't currently work this way we can postpone these changes to another PR, it's not an essential feature at the moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've updated the drawer so that it refetches the data every time it is shown, now it should be ok. I've also removed the aforementioned condition because of this change.

error.value.notificationTitle === t('error.cannot_edit_port_forward') ||
error.value.notificationTitle === t('error.cannot_create_port_forward')
) {
error.value.notificationTitle = ''
error.value.notificationDescription = ''
}
validationErrorBag.value.clear()
restrictIPValidationErrors.value = []

resetForm()
emit('close')
}

onMounted(() => {
fetchOptions()
firewallConfig.fetch()
if (firewallConfig.loading || firewallConfig.error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain this condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Considering how useFirewallStore is a global store and is used in different pages other than the port forward one, I thought it wouldn't make much sense to refetch its items when they already loaded, hence why i've included this condition. If you think this may cause issues I can remove it though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering how useFirewallStore is a global store and is used in different pages other than the port forward one, I thought it wouldn't make much sense to refetch its items when they already loaded, hence why i've included this condition. If you think this may cause issues I can remove it though!

Now it's clear, thanks

firewallConfig.fetch()
}
})

function resetRestrictIPValidationErrors() {
Expand Down Expand Up @@ -286,13 +298,17 @@ async function createOrEditPortForward() {
"
>
<NeInlineNotification
v-if="error.notificationTitle"
:title="error.notificationTitle"
:description="error.notificationDescription"
v-if="error.notificationTitle || firewallConfig.error"
:title="firewallConfig.error ? t('error.cannot_retrieve_zones') : error.notificationTitle"
:description="
firewallConfig.error
? t(getAxiosErrorMessage(firewallConfig.error))
: error.notificationDescription
"
class="mb-6"
kind="error"
/>
<NeSkeleton v-if="loading || firewallConfig.loading" :lines="10" />
<NeSkeleton v-if="loading || firewallConfig.loading || firewallConfig.error" :lines="10" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually don't show a skeleton if an error is displayed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, this is true but doing it this way could also cause a scenario where the user can interact with the form even if the firewallConfig fetch did not go as planned... How should we handle this case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, this is true but doing it this way could also cause a scenario where the user can interact with the form even if the firewallConfig fetch did not go as planned... How should we handle this case?

If an error occurs while retrieving drawer data we usually display only the error and hide the form

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, now if there's an error while fetching the firewall config the form does not show. I've also removed the condition from the skeleton.

<div v-else class="flex flex-col gap-y-6">
<NeToggle :label="t('standalone.port_forward.status')" v-model="enabled" />
<NeTextInput
Expand Down Expand Up @@ -327,13 +343,6 @@ async function createOrEditPortForward() {
></template
>
</NeTextInput>
<NeCombobox
:label="t('standalone.port_forward.destination_zone')"
:placeholder="t('standalone.port_forward.choose_zone')"
:options="supportedDestinationZones"
:selected-label="t('standalone.port_forward.any_zone')"
v-model="destinationZone"
/>
<NeTextInput
:label="t('standalone.port_forward.destination_port')"
v-model="destinationPort"
Expand All @@ -353,12 +362,18 @@ async function createOrEditPortForward() {
</NeButton>
</div>
<template v-if="showAdvancedSettings">
<NeCombobox
:label="t('standalone.port_forward.destination_zone')"
:placeholder="t('standalone.port_forward.choose_zone')"
:options="supportedDestinationZones"
:selected-label="t('standalone.port_forward.any_zone')"
v-model="destinationZone"
/>
<NeCombobox
:label="t('standalone.port_forward.wan_ip')"
:options="wanInterfaces"
v-model="wan"
/>

<NeMultiTextInput
:title="t('standalone.port_forward.restrict_access_to')"
:optional="true"
Expand Down
33 changes: 13 additions & 20 deletions src/views/standalone/firewall/PortForward.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {
NeButton,
NeTextInput,
NeSkeleton,
NeInlineNotification
NeInlineNotification,
NeEmptyState
} from '@nethserver/vue-tailwind-lib'
import PortForwardTable from '@/components/standalone/firewall/PortForwardTable.vue'
import { onMounted, ref } from 'vue'
Expand Down Expand Up @@ -204,34 +205,26 @@ onMounted(() => {
/>
<NeSkeleton v-if="loading" :lines="10" />
<template v-else>
<div
<NeEmptyState
:title="t('standalone.port_forward.no_port_forward_configured')"
:icon="['fas', 'circle-info']"
v-if="Object.keys(portForwards).length == 0"
class="flex flex-col items-center justify-center rounded-md p-10 dark:bg-gray-800"
>
<p class="mb-4 text-sm">
<strong>{{ t('standalone.port_forward.no_port_forward_configured') }}</strong>
</p>
<NeButton kind="primary" @click="openCreateEditDrawer(null)"
><NeButton kind="primary" @click="openCreateEditDrawer(null)"
><template #prefix>
<font-awesome-icon
:icon="['fas', 'circle-plus']"
class="h-4 w-4"
aria-hidden="true"
/> </template
>{{ t('standalone.port_forward.add_port_forward') }}</NeButton
>
</div>
<div
v-else-if="Object.keys(filteredPortForwards).length == 0"
class="flex flex-col items-center justify-center rounded-md p-10 dark:bg-gray-800"
></NeEmptyState
>
<p class="mb-4 text-sm">
<strong>{{ t('standalone.port_forward.no_port_forward_found') }}</strong>
</p>
<p class="text-sm">
{{ t('standalone.port_forward.filter_change_suggestion') }}
</p>
</div>
<NeEmptyState
:title="t('standalone.port_forward.no_port_forward_found')"
:description="t('standalone.port_forward.filter_change_suggestion')"
:icon="['fas', 'circle-info']"
v-else-if="Object.keys(filteredPortForwards).length == 0"
/>
<PortForwardTable
v-else
v-for="(portForward, key) in filteredPortForwards"
Expand Down