Add support for configuring WiFi devices by teclator · Pull Request #292 · agama-project/agama

teclator · 2022-11-03T08:25:43Z

Problem

In #260 we added basic support for configuring the network, the idea is to extend it supporting also wireless devices.

https://trello.com/c/SFpwJaU7/3139-5-d-installer-add-support-for-configuring-wifi-devices

Solution

We have added support for configuring WiFi networks at least allowing the user to connect to a network without security or using WPA-PSK authentication as well as configuring a Hidden network.

Testing

Added some unit tests (but it is still not well covered).
Tested manually

Screenshots

Click to show/hide some screenshots

Network summary with Wi-Fi capabilities	Available networks	Connecting to a network

Form for setting a connection	Form for setting a hidden connection	Detecting errors BEFORE trying to connect

* Extend the NetworkClient to retrieve the list of access points.

* It is just a draft and it does not work yet.

coveralls · 2022-11-03T08:29:12Z

Coverage decreased (-0.2%) to 74.68% when pulling 9c7715f on feature/wireless-support into bca356b on master.

It will encapsulate needed logic to create a connection object from given data before call to #addConnection method. Co-Authored-By: Knut Anderssen <kanderssen@suse.com>

imobachgs · 2022-11-16T12:42:05Z

web/src/KebabMenu.jsx

+import { Dropdown, DropdownToggle } from "@patternfly/react-core";
+import { EOS_MORE_VERT as MenuIcon } from "eos-icons-react";
+
+export default function KebabMenu({ items, position = "right", id = Date.now(), ...props }) {


I guess it might happen that you get the same id twice, right?

Yes, it could happen. However, it's difficult. Anyway, ideally the consumer will provide a better id, this is just the default/fallback value. I could change it for something like parseInt(Date.now() * Math.random()) if you think it worth it.

web/src/TargetIpsPopup.jsx

imobachgs · 2022-11-16T12:56:09Z

web/src/WifiConnectionForm.jsx

+
+const security_options = [
+  { value: "", label: "None" },
+  { value: "wpa-psk", label: "WPA & WPA2 Personal" }


I wonder if we should rely on SecurityProtocols. We do not need to fix it know, but we should review it along with other enums. Especially when translations come into play.

In this case we missed a class enum for adding things like the label for example.

Yes, that's true. My point is that using a "wpa-psk" looks fragile. But let's address it later.

imobachgs · 2022-11-16T12:56:52Z

web/src/WifiConnectionForm.jsx

+
+const securityFrom = (supported) => {
+  if (supported.includes("WPA2"))
+    return "wpa-psk";


Same as above: we are using an ad-hoc string when we already have an enum for this.

Similar to above and it also shows that is very related to the adapter... so maybe it should be declared there.

Sure it should not be declared here :-) but as said before, we can live with this.

web/src/WifiConnectionForm.jsx

imobachgs · 2022-11-16T13:09:07Z

web/src/WifiSelector.jsx

+  };
+
+  const renderFilteredNetworks = (networks) => {
+    return networks.map(n => {


Please, use a meaningful name. For a small function, is perfectly fine, but this one has around 60 lines :-)

imobachgs · 2022-11-16T13:11:15Z

web/src/WifiSelector.jsx

+
+  return (
+    <Popup isOpen={isOpen} height="large" title="Connect to a Wi-Fi network">
+      { renderFilteredNetworks(networksFromValues(networks)) }


Why not use a proper component? OK, I know, components are functions, but I would expect to have something like Networks and HiddenNetworkForm. Do you need to share all that state between them?

In any case, about the renderFilteredNetworks, does that function do any filtering? If that's not the case, I do not think you need to mention that in the name. It isn't very clear.

Imo, you're right.

Somehow, we forget to extract this to small components after so many changes to get the Wi-Fi selector working. I have tried to improve the situation a bit, but I fear a better improvement/re-organization must wait until next iterations in the network area.

What is more, I'd like to create a custom SelectionList component (Wi-Fi and Product selector are quite similar, though) and reorganize not only that part, but the whole src/ directory.

Hope, however, it looks good/acceptable enough for now.

imobachgs · 2022-11-16T13:15:24Z

web/src/WifiSelector.jsx

+          // Do not include networks without SSID
+          if (!ap.ssid || ap.ssid === "") return networks;
+          // Do not include "duplicates"
+          if (knownSsids.includes(ap.ssid)) return networks;


What about filtering networks you cannot configure? For instance, we cannot set up a WEP network, right? Alternatively, we could show them, but we should not allow trying to connect.

web/src/client/network.test.js

web/src/HiddenNetworkForm.jsx

imobachgs

It looks much better now. Just a few suggestions, but feel free to ignore them.

web/src/NetworkListItem.jsx

web/src/NetworksList.jsx

imobachgs

I have reviewed the tests more closely and found out that they use async too much. Additionally, if you do not need to access the useInstallerClient hook, you can use plainRender instead.

web/src/ConnectionsDataList.test.jsx

web/src/WifiHiddenNetworkForm.test.jsx

web/src/WifiNetworkListItem.test.jsx

web/src/WifiNetworksList.test.jsx

Improve ConnectionsDataList test

imobachgs and others added 19 commits November 3, 2022 08:21

[web] Add a model.js for network factories and types

13b2eb0

[web] Fix createConnection factory function

a18bf8e

[web] DBusClient#proxies waits for the proxies

2afdf92

[web] Extract the logic to convert the Connection to Cockpit

5d4d63b

[web] Add an AccessPoint model

b107871

* Extend the NetworkClient to retrieve the list of access points.

[web] Allow using destructuring to remove properties

271a730

[web] Extend the NetworkClient with an addConnection method

149df3e

[web] Avoid setting up twice a NetworkClient

8c1a42d

[web] Fix a typo in setUp

a772d07

[web] Added a wireless network selector

18a007e

* It is just a draft and it does not work yet.

Added AP supported security protocols function

12ceb13

Use a security attribute for the supported protocols

791df4e

[web] Move SecurityProtocols to model.js

000b50c

Added wireless security settings

d9da36d

Fix wireless config activation

6b7eb73

[web] Fix NetworkManagerAdapter tests

5fab6a4

[web] WirelessSelector returns null when no APs are available

118b438

[web] When on APs are available, do not display the link to connect

83bffe8

Fix filtering of ssids

70dbb4f

teclator changed the title ~~Feature/wireless support~~ Add support for configuring WiFi devices Nov 3, 2022

imobachgs and others added 7 commits November 3, 2022 08:53

[web] Use proxies to handle active connections

f663da1

[web] Simplify network events handling

7087ed2

[web] Methods to get the network config are not async anymore

e349042

[web] Fix the function to remove a handler

3ff6234

[web] Remove some unused @typedef

6418032

[web] Rename AP_* constants for consistency

54fd477

Added security form selector

95e655e

teclator force-pushed the feature/wireless-support branch from f443dc3 to 95e655e Compare November 7, 2022 09:05

[web] Add method for connect to a wireless network

d8aafd6

It will encapsulate needed logic to create a connection object from given data before call to #addConnection method. Co-Authored-By: Knut Anderssen <kanderssen@suse.com>

dgdavid force-pushed the feature/wireless-support branch from 93f2892 to b1f1dff Compare November 14, 2022 23:08

dgdavid mentioned this pull request Nov 14, 2022

[web] Display only one primary action when editing a connection #305

Merged

imobachgs linked an issue Nov 15, 2022 that may be closed by this pull request

Add support for configuring WiFi devices #262

Closed

imobachgs mentioned this pull request Nov 16, 2022

Release version 0.5.0 #310

Merged

Added unit tests to the network client (model and network_manager)

181adbc

teclator force-pushed the feature/wireless-support branch from e492b22 to 181adbc Compare November 16, 2022 13:16

imobachgs requested changes Nov 16, 2022

View reviewed changes

Fixed typo

a502fb1

teclator force-pushed the feature/wireless-support branch from bf1fc5a to 4119dbe Compare November 16, 2022 14:09

Moved formatIp function to the network client utils file

b89c9a7

teclator force-pushed the feature/wireless-support branch from 4119dbe to b89c9a7 Compare November 16, 2022 14:14

teclator and others added 2 commits November 16, 2022 18:26

Added unit test for ConnectionsDataList component

0f465ef

[web] Split WifiSelector in small components

beb0e4a

dgdavid reviewed Nov 16, 2022

View reviewed changes

web/src/HiddenNetworkForm.jsx Outdated Show resolved Hide resolved

imobachgs approved these changes Nov 16, 2022

View reviewed changes

web/src/NetworkListItem.jsx Outdated Show resolved Hide resolved

web/src/NetworksList.jsx Outdated Show resolved Hide resolved

web/src/NetworksList.jsx Outdated Show resolved Hide resolved

dgdavid and others added 4 commits November 17, 2022 00:18

[web] Improvements from code review

71f5d59

[web] Rename some Wi-Fi related components

768d6e9

[web] Add classNames utility

4491ee2

[web] Improve the ConnectionsDataList

65d5b6e

imobachgs requested changes Nov 17, 2022

View reviewed changes

imobachgs and others added 4 commits November 17, 2022 08:32

Merge pull request #313 from yast/improve-test

fbd1ec4

Improve ConnectionsDataList test

Do not use async when not needed (CR suggestion)

bc38096

Added unit test for Network component

146bdfd

Added a Fixme for moving the security options

9c7715f

imobachgs approved these changes Nov 17, 2022

View reviewed changes

dgdavid merged commit b044a44 into master Nov 17, 2022

dgdavid deleted the feature/wireless-support branch November 17, 2022 13:58

teclator mentioned this pull request Nov 17, 2022

Only show the link for connecting to a WiFI network if wireless is enabled #316

Merged

imobachgs restored the feature/wireless-support branch November 23, 2022 09:04

imobachgs deleted the feature/wireless-support branch November 23, 2022 09:05

Conversation

teclator commented Nov 3, 2022 • edited by dgdavid Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Testing

Screenshots

Uh oh!

coveralls commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

imobachgs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imobachgs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

teclator commented Nov 3, 2022 •

edited by dgdavid

Loading

coveralls commented Nov 3, 2022 •

edited

Loading

imobachgs left a comment •

edited

Loading