From 653359d815ce4113ff384848e3c0dd5b430bb1e7 Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Thu, 26 Aug 2021 12:57:40 +0200
Subject: [PATCH] [libcalamares] Fix up multiple URLs for checkinternet

- was filtering out the wrong URLs
- was not actually removing the invalid URLs
- extend API to make it possible to count / confirm the settings
- extend tests to demonstrate that API and the issues
---
 src/libcalamares/network/Manager.cpp | 14 ++++++++++++--
 src/libcalamares/network/Manager.h   |  4 ++++
 src/libcalamares/network/Tests.cpp   | 20 ++++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/src/libcalamares/network/Manager.cpp b/src/libcalamares/network/Manager.cpp
index 0e651f45c..5125f592e 100644
--- a/src/libcalamares/network/Manager.cpp
+++ b/src/libcalamares/network/Manager.cpp
@@ -218,8 +218,12 @@ Manager::setCheckHasInternetUrl( const QVector< QUrl >& urls )
 {
     d->m_lastCheckedUrlIndex = -1;
     d->m_hasInternetUrls = urls;
-    std::remove_if(
-        d->m_hasInternetUrls.begin(), d->m_hasInternetUrls.end(), []( const QUrl& u ) { return u.isValid(); } );
+    auto it = std::remove_if(
+        d->m_hasInternetUrls.begin(), d->m_hasInternetUrls.end(), []( const QUrl& u ) { return !u.isValid(); } );
+    if ( it != d->m_hasInternetUrls.end() )
+    {
+        d->m_hasInternetUrls.erase( it );
+    }
 }
 
 void
@@ -231,6 +235,12 @@ Manager::addCheckHasInternetUrl( const QUrl& url )
     }
 }
 
+QVector< QUrl >
+Manager::getCheckInternetUrls() const
+{
+    return d->m_hasInternetUrls;
+}
+
 /** @brief Does a request asynchronously, returns the (pending) reply
  *
  * The extra options for the request are taken from @p options,
diff --git a/src/libcalamares/network/Manager.h b/src/libcalamares/network/Manager.h
index 8bc3dded7..6a906c883 100644
--- a/src/libcalamares/network/Manager.h
+++ b/src/libcalamares/network/Manager.h
@@ -90,6 +90,7 @@ class DLLEXPORT Manager : public QObject
 {
     Q_OBJECT
     Q_PROPERTY( bool hasInternet READ hasInternet NOTIFY hasInternetChanged FINAL )
+    Q_PROPERTY( QVector< QUrl > checkInternetUrls READ getCheckInternetUrls WRITE setCheckHasInternetUrl )
 
     Manager();
 
@@ -129,6 +130,9 @@ public:
     /// @brief Set a collection of URLs used for the general "is there internet" check.
     void setCheckHasInternetUrl( const QVector< QUrl >& urls );
 
+    /// @brief What URLs are used to check for internet connectivity?
+    QVector< QUrl > getCheckInternetUrls() const;
+
     /** @brief Do a network request asynchronously.
      *
      * Returns a pointer to the reply-from-the-request.
diff --git a/src/libcalamares/network/Tests.cpp b/src/libcalamares/network/Tests.cpp
index d44f03781..a5bc52497 100644
--- a/src/libcalamares/network/Tests.cpp
+++ b/src/libcalamares/network/Tests.cpp
@@ -30,6 +30,7 @@ NetworkTests::testInstance()
 {
     auto& nam = CalamaresUtils::Network::Manager::instance();
     QVERIFY( !nam.hasInternet() );
+    QCOMPARE( nam.getCheckInternetUrls().count(), 0 );
 }
 
 void
@@ -73,18 +74,21 @@ NetworkTests::testCheckUrl()
         QVERIFY( u.isValid() );
         nam.setCheckHasInternetUrl( u );
         QVERIFY( nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 1 );  // Valid URL
     }
     {
         QUrl u( "http://nonexistent.example.com" );
         QVERIFY( u.isValid() );
         nam.setCheckHasInternetUrl( u );
         QVERIFY( !nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 1 );  // Valid URL even if it doesn't resolve
     }
     {
         QUrl u;
         QVERIFY( !u.isValid() );
         nam.setCheckHasInternetUrl( u );
         QVERIFY( !nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 0 );  // Invalid URL tried
     }
 }
 
@@ -102,6 +106,7 @@ NetworkTests::testCheckMultiUrl()
         QVERIFY( u1.isValid() );
         nam.setCheckHasInternetUrl( { u0, u1 } );
         QVERIFY( nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 2 );
     }
     {
         QUrl u0( "http://nonexistent.example.com" );
@@ -111,7 +116,22 @@ NetworkTests::testCheckMultiUrl()
         nam.setCheckHasInternetUrl( { u0, u1 } );
         QVERIFY( !nam.checkHasInternet() );
         QVERIFY( !nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 2 );  // Both are valid URLs
         nam.addCheckHasInternetUrl( QUrl( "http://example.com" ) );
         QVERIFY( nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 3 );
+    }
+    {
+        QUrl u0( "http://nonexistent.example.com" );
+        QUrl u1;
+        QVERIFY( u0.isValid() );
+        QVERIFY( !u1.isValid() );
+        nam.setCheckHasInternetUrl( { u0, u1 } );
+        QVERIFY( !nam.checkHasInternet() );
+        QVERIFY( !nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 1 );  // Only valid URL added
+        nam.addCheckHasInternetUrl( QUrl( "http://example.com" ) );
+        QVERIFY( nam.checkHasInternet() );
+        QCOMPARE( nam.getCheckInternetUrls().count(), 2 );
     }
 }
-- 
2.11.0