feat(xo-lite/network): implementation of network tab and add network/pif store

[J0ris-K]

Description

Implementation of network tab in pool and add network/pifs store to get data

### Screenshot

Checklist

• Commit
  • Title follows commit conventions
  • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
  • If bug fix, add Introduced by
• Changelog
  • If visible by XOA users, add changelog entry
  • Update "Packages to release" in CHANGELOG.unreleased.md
• PR
  • If UI changes, add screenshots
  • If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

• develop skills of junior reviewers
• limit the workload for senior reviewers
• limit the number of unnecessary changes by the author

1. The author creates a PR.
2. Review process:
   1. The author assigns the junior reviewer.
   2. The junior reviewer conducts their review:
      • Resolves their comments if they are addressed.
      • Adds comments if necessary or approves the PR.
   3. The junior reviewer assigns the senior reviewer.
   4. The senior reviewer conducts their review:
      • If there are no unresolved comments on the PR → merge.
      • Otherwise, we continue with 3.
3. The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

1. The author can request a review at any time, even if the PR is still a Draft.
2. In theory, there should not be more than one reviewer at a time.
3. The author should not make any changes:
   • When a reviewer is assigned.
   • Between the junior and senior reviews.

[OlivierFL]

Same here, maybe the wording can be improved?
BTW, some typos need to be fixed:

Suggested change

	"locking-mode": "Mode verouillé",
	"locking-mode-default": "Mode verouillé par defaut",
	"locking-mode": "Mode verrouillé",
	"locking-mode-default": "Mode verrouillé par défaut",

[J0ris-K]

Any suggestion ? 😃

[OlivierFL]

This <tr> can be extracted into a child component, for example, a PifRow.vue component, that will accept a PIF object as a prop and will handle the styling and other things if needed.
See this comment for more explanation, even if it is for XO 6, the song concept remains the same.

[OlivierFL]

Be sure to keep blank lines in translation files. In WebStorm, set keep maximum blank lines to 1:

[OlivierFL]

By convention, I think it would be better to use the kebab-case here:

Suggested change

	pif: { status: 'connected' | 'disconnected' | 'partially_connected' }
	pif: { status: 'connected' | 'disconnected' | 'partially-connected' }

Remember to update the rest of the code and the usages accordingly.

[OlivierFL]

Be careful when updating the styles of core UI components. This property is useful here to keep the icon properly aligned when the text in the UiInfo is more than one line long (example here).
However, I agree that in your case, the icon alignment could be better. You can add a class in PoolNetworksPifStatus and use it to change the alignment, see my comment on the component.

[J0ris-K]

I agree to be careful when updating core UI components.
But shouldn't we remove align-items: start and use if we need it? I have a feeling we'll need align-items: center more often. Since it's the default behavior, no ?

[OlivierFL]

Yes, maybe that would be better to handle single line info by default and add an override if necessary. Or we can add a prop for that? @ByScripts

[OlivierFL]

Based on my comment on UiInfo, you can add a class and change the icon alignment here:

<template>
  <UiInfo class="pif-status">
</template>

<style lang="postcss" scoped>
.pif-status {
  align-items: center;
}
</style>

[ByScripts]

Suggested change

	const { status } = defineProps<{
	status: 'connected' | 'disconnected' | 'partial'
	}>()
	const { t } = useI18n()
	type NetworkAccent = 'success' | 'warning' | 'danger'
	const statusMap: Record<string, { text: string; accent: NetworkAccent }> = {
	connected: { text: t('connected'), accent: 'success' },
	disconnected: { text: t('disconnected'), accent: 'danger' },
	partial: { text: t('partially-connected'), accent: 'warning' },
	}
	type Status = 'connected' | 'disconnected' | 'partial'
	type Accent = 'success' | 'warning' | 'danger'
	const { status } = defineProps<{
	status: Status
	}>()
	const { t } = useI18n()
	const statusMap: Record<Status, { text: string; accent: Accent }> = {
	connected: { text: t('connected'), accent: 'success' },
	disconnected: { text: t('disconnected'), accent: 'danger' },
	partial: { text: t('partially-connected'), accent: 'warning' },
	}

[ByScripts]

I don't think this is necessary. We can use the prop directly.

[ByScripts]

I'd suggest simplifying the naming.

Also, remember to use plural if expected data is an array.

Suggested change

	const props = defineProps<{
	hostInternalNetwork: XenApiNetwork[]
	isReady: boolean
	}>()
	const { networks, isReady } = defineProps<{
	networks: XenApiNetwork[]
	isReady: boolean
	}>()

[ByScripts]

I'd suggest something like that for better readability:

Suggested change

	const filteredNetworks = computed(() => {
	return searchQuery.value
	? reactiveHostInternalNetworks.value.filter(network =>
	Object.values(network).some(value => String(value).toLowerCase().includes(searchQuery.value.toLowerCase()))
	)
	: reactiveHostInternalNetworks.value
	})
	const filteredNetworks = computed(() => {
	const searchTerm = searchQuery.value.trim().toLocaleLowerCase()
	if (!searchTerm) {
	return networks.value
	}
	return networks.value.filter(network =>
	Object.values(network).some(value => String(value).toLocaleLowerCase().includes(searchTerm))
	)
	})```

[ByScripts]

Suggested change

	const usableRefs = computed(() => reactiveHostInternalNetworks.value.map(item => item.uuid))
	const { selected, areAllSelected } = useMultiSelect(usableRefs)
	const networkUuids = computed(() => networks.value.map(network => network.uuid))
	const { selected, areAllSelected } = useMultiSelect(networkUuids)

[ByScripts]

Thanks to the mapping of useTable, column already contains the value of the cell, so you can do something like this:

Suggested change

	<td v-for="column of row.visibleColumns" :key="column.id" class="typo p2-regular">
	<UiCheckbox v-if="column.id === 'checkbox'" v-model="selected" accent="info" :value="row.id" />
	<!-- NEED TO REMOVE `as XenApiNetwork` -->
	<div
	v-if="column.id === 'name_label' && (row.value as XenApiNetwork).name_label"
	v-tooltip="{ placement: 'bottom-end' }"
	class="text-ellipsis"
	>
	{{ (row.value as XenApiNetwork).name_label }}
	</div>
	<div
	v-if="column.id === 'name_description'"
	v-tooltip="{ placement: 'bottom-end' }"
	class="text-ellipsis"
	>
	{{ (row.value as XenApiNetwork).name_description }}
	</div>
	<div v-if="column.id === 'MTU'" v-tooltip="{ placement: 'bottom-end' }" class="text-ellipsis">
	{{ (row.value as XenApiNetwork).MTU }}
	</div>
	<div
	v-if="column.id === 'default_locking_mode'"
	v-tooltip="{ placement: 'bottom-end' }"
	class="text-ellipsis"
	>
	{{ (row.value as XenApiNetwork).default_locking_mode }}
	</div>
	<div v-if="column.id === 'more'">
	<VtsIcon accent="info" :icon="faEllipsis" />
	</div>
	</td>
	<td v-for="column of row.visibleColumns" :key="column.id" class="typo p2-regular">
	<UiCheckbox v-if="column.id === 'checkbox'" v-model="selected" accent="info" :value="row.id" />
	<VtsIcon v-else-if="column.id === 'more'" accent="info" :icon="faEllipsis" />
	<div
	v-else
	v-tooltip="{ placement: 'bottom-end' }"
	class="text-ellipsis"
	>
	{{ column.value }}
	</div>
	</td>

Another possibility is to remove "checkbox" and "more" columns from column definitions and add them manually:

Suggested change

	<td v-for="column of row.visibleColumns" :key="column.id" class="typo p2-regular">
	<UiCheckbox v-if="column.id === 'checkbox'" v-model="selected" accent="info" :value="row.id" />
	<!-- NEED TO REMOVE `as XenApiNetwork` -->
	<div
	v-if="column.id === 'name_label' && (row.value as XenApiNetwork).name_label"
	v-tooltip="{ placement: 'bottom-end' }"
	class="text-ellipsis"
	>
	{{ (row.value as XenApiNetwork).name_label }}
	</div>
	<div
	v-if="column.id === 'name_description'"
	v-tooltip="{ placement: 'bottom-end' }"
	class="text-ellipsis"
	>
	{{ (row.value as XenApiNetwork).name_description }}
	</div>
	<div v-if="column.id === 'MTU'" v-tooltip="{ placement: 'bottom-end' }" class="text-ellipsis">
	{{ (row.value as XenApiNetwork).MTU }}
	</div>
	<div
	v-if="column.id === 'default_locking_mode'"
	v-tooltip="{ placement: 'bottom-end' }"
	class="text-ellipsis"
	>
	{{ (row.value as XenApiNetwork).default_locking_mode }}
	</div>
	<div v-if="column.id === 'more'">
	<VtsIcon accent="info" :icon="faEllipsis" />
	</div>
	</td>
	<td>
	<UiCheckbox v-model="selected" accent="info" :value="row.id" />
	</td>
	<td v-for="column of row.visibleColumns" :key="column.id" class="typo p2-regular">
	<div v-tooltip="{ placement: 'bottom-end' }" class="text-ellipsis">
	{{ column.value }}
	</div>
	</td>
	<td>
	<VtsIcon accent="info" :icon="faEllipsis" />
	</td>

[J0ris-K]

Wonderful !

[ByScripts]

Types should use PascalCase

Suggested change

	type networkHeader = 'name_label' | 'name_description' | 'MTU' | 'default_locking_mode' | 'more'
	const headerIcon: Record<networkHeader, { icon: IconDefinition }> = {
	type NetworkHeader = 'name_label' | 'name_description' | 'MTU' | 'default_locking_mode' | 'more'
	const headerIcon: Record<NetworkHeader, { icon: IconDefinition }> = {

[ByScripts]

Unneeded

[ByScripts]

Same as previous comment concerning the columns of useTable

[ByScripts]

Unneeded. Data is already reactive.

Suggested change

	const { networksWithVLANs, hostInternalNetworks, isReady } = useNetworkStore().subscribe()
	const reactiveNetworksWithVLANs = ref(networksWithVLANs.value || [])
	const reactiveHostInternalNetworks = ref(hostInternalNetworks.value || [])
	watchEffect(() => {
	if (networksWithVLANs.value) {
	reactiveNetworksWithVLANs.value = networksWithVLANs.value || []
	}
	if (hostInternalNetworks.value) {
	reactiveHostInternalNetworks.value = hostInternalNetworks.value || []
	}
	})
	</script>
	const { networksWithVLANs, hostInternalNetworks, isReady } = useNetworkStore().subscribe()
	</script>

[ByScripts]

Why does that prop turn optional?

In case it'd be legitimate, then undefined status should be handled properly somewhere.

[ByScripts]

Exclamation point in TypeScript should be used with caution.

It means "You, as TypeScript, are not able to understand that this variable is defined. But me, as a developer, can assert that it is."

In this case, since the prop is optional, you can't assert that it will be defined, so you can't use an exclamation point here.

Here is an example where ! would be useful:

<template>
  <div ref="elm" />
</template>

<script lang="ts" setup>
const elm = ref<HTMLELement | null>(null)

onMounted(() => {
  doSomething(elm.value!) // <= Here, we tell TypeScript that we're sure this value is set, although TS can't guess.
})
</script>

[J0ris-K]

ok, thanks for the explanation!

[ByScripts]

Typing is already inferred here.

Suggested change

	<UiQuerySearchBar class="table-query" @search="(value: string) => (searchQuery = value)" />
	<UiQuerySearchBar class="table-query" @search="value => (searchQuery = value)" />

[ByScripts]

You can't use an fixed id inside a v-for loop.

[J0ris-K]

Yes, it's a mistake

[ByScripts]

Is this necessary to wrap PoolNetworksPifStatus in a div?

Also, that div has a status class, but there is no .status selector in the CSS.

[J0ris-K]

No it's not necessary, it seems to be an oversight :/

[ByScripts]

There's a little naming problem here.

Having a networks prop containing an array of objects where each object has a network property is really confusing.

It leads to code like network.network.<...>) which looks wrong.

[J0ris-K]

Yes.. indeed

[J0ris-K]

Do you have any suggestions ?

[ByScripts]

Suggested change

	const headerIcon: Record<NetworkHeader, { icon: IconDefinition }> = {
	name_label: { icon: faAlignLeft },
	name_description: { icon: faAlignLeft },
	status: { icon: faPowerOff },
	vlan: { icon: faAlignLeft },
	MTU: { icon: faHashtag },
	default_locking_mode: { icon: faCaretDown },
	more: { icon: faEllipsis },
	}
	const getHeaderIcon = (status: NetworkHeader) => headerIcon[status].icon
	const headerIcon: Record<NetworkHeader, IconDefinition> = {
	name_label: faAlignLeft,
	name_description: faAlignLeft,
	status: faPowerOff,
	vlan: faAlignLeft,
	MTU: faHashtag,
	default_locking_mode: faCaretDown,
	more: faEllipsis,
	}
	const getHeaderIcon = (status: NetworkHeader) => headerIcon[status]

[ByScripts]

Let's keep consistency in variable names. (pifContext/PIFsByNetwork)

Suggested change

	const PIFsByNetwork = computed(() => {
	const pifsByNetwork = computed(() => {

[ByScripts]

Redundant typing should be extracted to a type and be reused.

Suggested change

	{
	network: XenApiNetwork
	vlan: string
	status: Status
	}
	{
	network: XenApiNetwork
	vlan: string
	status: Status
	}

[ByScripts]

This part is really not clear.

First, you do the same thing inside the conditional statement than after it.

Also, what is the whole point of the networksInfoMap here? You store data in it but never seem to use that information anywhere else.

[J0ris-K]

Yes, indeed, it wasn't clear to me at first, so I did several tests and because it worked, nonsense like that remained

[ByScripts]

Since you have a pif store, why did you put this in the network store?

Suggested change

	function determineStatus(PIFs: XenApiPif[]): Status {
	if (PIFs.length === 0) {
	return 'disconnected'
	}
	const currentlyAttached = PIFs.map(PIF => PIF.currently_attached)
	if (currentlyAttached.every(Boolean)) {
	return 'connected'
	}
	if (currentlyAttached.some(Boolean)) {
	return 'partial'
	}
	return 'disconnected'
	}
	function determineStatus(PIFs: XenApiPif[]): Status {
	if (PIFs.length === 0) {
	return 'disconnected'
	}
	const currentlyAttached = PIFs.map(PIF => PIF.currently_attached)
	if (currentlyAttached.every(Boolean)) {
	return 'connected'
	}
	if (currentlyAttached.some(Boolean)) {
	return 'partial'
	}
	return 'disconnected'
	}