Merge pull request #55284 from elpaso/feed-enhancements

NewsFeed enhancements

.github/workflows/code_layout.yml

@@ -157,7 +157,7 @@ jobs:
         with:
           separator: ' '
       - name: Spell Test
-        run: ./scripts/spell_check/check_spelling.sh -r ${{ steps.changed_files.outputs.all_changed_files }}
+        run: ./scripts/spell_check/check_spelling.sh -r "${{ steps.changed_files.outputs.all_changed_files }}"
 
   sip_check:
     runs-on: ubuntu-latest

python/PyQt6/core/auto_generated/network/qgsnewsfeedparser.sip.in

@@ -109,7 +109,7 @@ Fetches new entries from the feed's URL.
 
     void fetched( const QList< QgsNewsFeedParser::Entry > &entries );
 %Docstring
-Emitted when ``entries`` have fetched from the feed.
+Emitted when ``entries`` have been fetched from the feed.
 
 .. seealso:: :py:func:`fetch`
 %End
@@ -120,6 +120,16 @@ Emitted whenever a new ``entry`` is available from the feed (as a result
 of a call to :py:func:`~QgsNewsFeedParser.fetch`).
 
 .. seealso:: :py:func:`fetch`
+%End
+
+    void entryUpdated( const QgsNewsFeedParser::Entry &entry );
+%Docstring
+Emitted whenever an existing ``entry`` is available from the feed (as a result
+of a call to :py:func:`~QgsNewsFeedParser.fetch`).
+
+.. seealso:: :py:func:`fetch`
+
+.. versionadded:: 3.36
 %End
 
     void entryDismissed( const QgsNewsFeedParser::Entry &entry );

python/core/auto_generated/network/qgsnewsfeedparser.sip.in

@@ -109,7 +109,7 @@ Fetches new entries from the feed's URL.
 
     void fetched( const QList< QgsNewsFeedParser::Entry > &entries );
 %Docstring
-Emitted when ``entries`` have fetched from the feed.
+Emitted when ``entries`` have been fetched from the feed.
 
 .. seealso:: :py:func:`fetch`
 %End
@@ -120,6 +120,16 @@ Emitted whenever a new ``entry`` is available from the feed (as a result
 of a call to :py:func:`~QgsNewsFeedParser.fetch`).
 
 .. seealso:: :py:func:`fetch`
+%End
+
+    void entryUpdated( const QgsNewsFeedParser::Entry &entry );
+%Docstring
+Emitted whenever an existing ``entry`` is available from the feed (as a result
+of a call to :py:func:`~QgsNewsFeedParser.fetch`).
+
+.. seealso:: :py:func:`fetch`
+
+.. versionadded:: 3.36
 %End
 
     void entryDismissed( const QgsNewsFeedParser::Entry &entry );

scripts/spell_check/.agignore

@@ -11,6 +11,7 @@ python/ext-libs/
 python/qsci_apis/
 src/core/pal
 tests/testdata/provider/postgresraster/
+tests/testdata/newsfeed/
 
 #Extensions
 *.*.orig

src/core/network/qgsnewsfeedmodel.cpp

@@ -31,6 +31,7 @@ QgsNewsFeedModel::QgsNewsFeedModel( QgsNewsFeedParser *parser, QObject *parent )
 
   connect( mParser, &QgsNewsFeedParser::entryAdded, this, &QgsNewsFeedModel::onEntryAdded );
   connect( mParser, &QgsNewsFeedParser::entryDismissed, this, &QgsNewsFeedModel::onEntryRemoved );
+  connect( mParser, &QgsNewsFeedParser::entryUpdated, this, &QgsNewsFeedModel::onEntryUpdated );
   connect( mParser, &QgsNewsFeedParser::imageFetched, this, &QgsNewsFeedModel::onImageFetched );
 }
 
@@ -120,6 +121,19 @@ void QgsNewsFeedModel::onEntryAdded( const QgsNewsFeedParser::Entry &entry )
   endInsertRows();
 }
 
+void QgsNewsFeedModel::onEntryUpdated( const QgsNewsFeedParser::Entry &entry )
+{
+  for ( int idx = 0; idx < mEntries.count(); idx++ )
+  {
+    if ( mEntries.at( idx ).key == entry.key )
+    {
+      mEntries[ idx ] = entry;
+      emit dataChanged( index( idx, 0 ), index( idx, 0 ) );
+      break;
+    }
+  }
+}
+
 void QgsNewsFeedModel::onEntryRemoved( const QgsNewsFeedParser::Entry &entry )
 {
   // find index of entry

src/core/network/qgsnewsfeedmodel.h

@@ -68,6 +68,7 @@ class CORE_EXPORT QgsNewsFeedModel : public QAbstractItemModel
 
     void onEntryAdded( const QgsNewsFeedParser::Entry &entry );
     void onEntryRemoved( const QgsNewsFeedParser::Entry &entry );
+    void onEntryUpdated( const QgsNewsFeedParser::Entry &entry );
     void onImageFetched( int key, const QPixmap &pixmap );
 
   private:

src/core/network/qgsnewsfeedparser.cpp

@@ -190,33 +190,73 @@ void QgsNewsFeedParser::onFetch( const QString &content )
   const QVariant json = QgsJsonUtils::parseJson( content );
 
   const QVariantList entries = json.toList();
-  QList< QgsNewsFeedParser::Entry > newEntries;
-  newEntries.reserve( entries.size() );
+  QList< QgsNewsFeedParser::Entry > fetchedEntries;
+  fetchedEntries.reserve( entries.size() );
   for ( const QVariant &e : entries )
   {
-    Entry newEntry;
+    Entry incomingEntry;
     const QVariantMap entryMap = e.toMap();
-    newEntry.key = entryMap.value( QStringLiteral( "pk" ) ).toInt();
-    newEntry.title = entryMap.value( QStringLiteral( "title" ) ).toString();
-    newEntry.imageUrl = entryMap.value( QStringLiteral( "image" ) ).toString();
-    newEntry.content = entryMap.value( QStringLiteral( "content" ) ).toString();
-    newEntry.link = entryMap.value( QStringLiteral( "url" ) ).toString();
-    newEntry.sticky = entryMap.value( QStringLiteral( "sticky" ) ).toBool();
-    bool ok = false;
-    const uint expiry = entryMap.value( QStringLiteral( "publish_to" ) ).toUInt( &ok );
-    if ( ok )
-      newEntry.expiry.setSecsSinceEpoch( expiry );
-    newEntries.append( newEntry );
+    incomingEntry.key = entryMap.value( QStringLiteral( "pk" ) ).toInt();
+    incomingEntry.title = entryMap.value( QStringLiteral( "title" ) ).toString();
+    incomingEntry.imageUrl = entryMap.value( QStringLiteral( "image" ) ).toString();
+    incomingEntry.content = entryMap.value( QStringLiteral( "content" ) ).toString();
+    incomingEntry.link = entryMap.value( QStringLiteral( "url" ) ).toString();
+    incomingEntry.sticky = entryMap.value( QStringLiteral( "sticky" ) ).toBool();
+    bool hasExpiry = false;
+    const qlonglong expiry = entryMap.value( QStringLiteral( "publish_to" ) ).toLongLong( &hasExpiry );
+    if ( hasExpiry )
+      incomingEntry.expiry.setSecsSinceEpoch( expiry );
 
-    if ( !newEntry.imageUrl.isEmpty() )
-      fetchImageForEntry( newEntry );
+    fetchedEntries.append( incomingEntry );
+
+    // We also need to handle the case of modified/expired entries
+    const auto entryIter { std::find_if( mEntries.begin(), mEntries.end(), [incomingEntry]( const QgsNewsFeedParser::Entry & candidate )
+    {
+      return candidate.key == incomingEntry.key;
+    } )};
+    const bool entryExists { entryIter != mEntries.end() };
+
+    // case 1: existing entry is now expired, dismiss
+    if ( hasExpiry && expiry < mFetchStartTime )
+    {
+      dismissEntry( incomingEntry.key );
+    }
+    // case 2: existing entry edited
+    else if ( entryExists )
+    {
+      const bool imageNeedsUpdate = ( entryIter->imageUrl != incomingEntry.imageUrl );
+      // also remove preview image, if it exists
+      if ( imageNeedsUpdate && ! entryIter->imageUrl.isEmpty() )
+      {
+        const QString previewDir = QStringLiteral( "%1/previewImages" ).arg( QgsApplication::qgisSettingsDirPath() );
+        const QString imagePath = QStringLiteral( "%1/%2.png" ).arg( previewDir ).arg( entryIter->key );
+        if ( QFile::exists( imagePath ) )
+        {
+          QFile::remove( imagePath );
+        }
+      }
+      *entryIter = incomingEntry;
+      if ( imageNeedsUpdate && ! incomingEntry.imageUrl.isEmpty() )
+        fetchImageForEntry( incomingEntry );
+
+      sTreeNewsFeedEntries->deleteItem( QString::number( incomingEntry.key ), {mFeedKey} );
+      storeEntryInSettings( incomingEntry );
+      emit entryUpdated( incomingEntry );
+    }
+    // else: new entry, not expired
+    else if ( !hasExpiry || expiry >= mFetchStartTime )
+    {
+      if ( !incomingEntry.imageUrl.isEmpty() )
+        fetchImageForEntry( incomingEntry );
+
+      mEntries.append( incomingEntry );
+      storeEntryInSettings( incomingEntry );
+      emit entryAdded( incomingEntry );
+    }
 
-    mEntries.append( newEntry );
-    storeEntryInSettings( newEntry );
-    emit entryAdded( newEntry );
   }
 
-  emit fetched( newEntries );
+  emit fetched( fetchedEntries );
 }
 
 void QgsNewsFeedParser::readStoredEntries()

src/core/network/qgsnewsfeedparser.h

@@ -145,7 +145,7 @@ class CORE_EXPORT QgsNewsFeedParser : public QObject
   signals:
 
     /**
-     * Emitted when \a entries have fetched from the feed.
+     * Emitted when \a entries have been fetched from the feed.
      *
      * \see fetch()
      */
@@ -159,6 +159,16 @@ class CORE_EXPORT QgsNewsFeedParser : public QObject
      */
     void entryAdded( const QgsNewsFeedParser::Entry &entry );
 
+    /**
+     * Emitted whenever an existing \a entry is available from the feed (as a result
+     * of a call to fetch()).
+     *
+     * \see fetch()
+     *
+     * \since QGIS 3.36
+     */
+    void entryUpdated( const QgsNewsFeedParser::Entry &entry );
+
     /**
      * Emitted whenever an \a entry is dismissed (as a result of a call
      * to dismissEntry()).

tests/src/core/testqgsnewsfeedparser.cpp

@@ -39,6 +39,7 @@ class TestQgsNewsFeedParser: public QObject
     void testGeoFencing();
     void testModel();
     void testProxyModel();
+    void testUpdatedEntries();
 
 };
 
@@ -410,6 +411,68 @@ void TestQgsNewsFeedParser::testProxyModel()
   QCOMPARE( model2.data( model2.index( 3, 0, QModelIndex() ), QgsNewsFeedModel::Title ).toString(), QStringLiteral( "QGIS acquired by ESRI" ) );
 }
 
+void TestQgsNewsFeedParser::testUpdatedEntries()
+{
+  QList< QgsNewsFeedParser::Entry > entries;
+
+  const QUrl url( QUrl::fromLocalFile( QStringLiteral( TEST_DATA_DIR ) + "/newsfeed/feed" ) );
+  const QString feedKey = QgsNewsFeedParser::keyForFeed( url.toString() );
+
+  // reset to a standard known last time
+  QgsNewsFeedParser::sTreeNewsFeed->deleteItem( feedKey );
+  QgsNewsFeedParser::settingsFeedLastFetchTime->setValue( 1457360008, feedKey );
+
+  // refetch, only new items should be fetched
+  QgsNewsFeedParser parser( url );
+  QVERIFY( parser.entries().isEmpty() );
+  QEventLoop loop;
+  connect( &parser, &QgsNewsFeedParser::fetched, this, [ =, &loop, &entries ]( const  QList< QgsNewsFeedParser::Entry > &e )
+  {
+    entries = e;
+    loop.quit();
+  } );
+
+  parser.fetch();
+  loop.exec();
+
+  // check only new entries are present
+  QCOMPARE( entries.count(), 4 );
+
+  entries.clear();
+
+  // Now request a new date and check that:
+  // - entry 3 has a new expiry date in the past so it is removed
+  // - entry 11 has been modified
+
+  QgsNewsFeedParser::settingsFeedLastFetchTime->setValue( 1557079653, feedKey );
+
+  QgsNewsFeedParser parser2( url );
+  QVERIFY( ! parser2.entries().isEmpty() );
+  QEventLoop loop2;
+  connect( &parser2, &QgsNewsFeedParser::fetched, this, [ =, &loop2, &entries ]( const  QList< QgsNewsFeedParser::Entry > &e )
+  {
+    entries = e;
+    loop2.quit();
+  } );
+
+  const QSignalSpy spyUpdated( &parser2, &QgsNewsFeedParser::entryUpdated );
+  const QSignalSpy spyDismissed( &parser2, &QgsNewsFeedParser::entryDismissed );
+
+  parser2.fetch();
+  loop2.exec();
+
+  QCOMPARE( spyDismissed.count(), 1 );
+  QCOMPARE( spyUpdated.count(), 1 );
+
+  QCOMPARE( entries.count(), 2 );
+  QCOMPARE( parser2.entries().count(), 3 );
+  QCOMPARE( parser2.entries().at( 0 ).title, QStringLiteral( "Next Microsoft Windows code name revealed" ) );
+  QCOMPARE( parser2.entries().at( 1 ).title, QStringLiteral( "Null Island QGIS Meeting" ) );
+  QCOMPARE( parser2.entries().at( 2 ).title, QStringLiteral( "QGIS Italian Meeting Revisited" ) );
+  QCOMPARE( parser2.entries().at( 2 ).expiry.toSecsSinceEpoch(), 7868426853 );
+
+}
+
 
 QGSTEST_MAIN( TestQgsNewsFeedParser )
 #include "testqgsnewsfeedparser.moc"

tests/testdata/newsfeed/feed_after=1557079653&lang=en

@@ -0,0 +1,22 @@
+[
+  {
+    "pk": 3,
+    "publish_from": 1557073748.136,
+    "publish_to":   1557078653,
+    "title": "QGIS acquired by ESRI",
+    "image": "",
+    "content": "<p>QGIS is finally part of the ESRI ecosystem, it has been rebranded as CrashGIS to better integrate with ESRI products line.</p>",
+    "url": "https://www.qgis.com",
+    "sticky": false
+  },
+  {
+    "pk": 11,
+    "publish_from": 1557360008.132,
+    "publish_to":   7868426853,
+    "title": "QGIS Italian Meeting Revisited",
+    "image": "",
+    "content": "<p>Hello from Italy!</p>",
+    "url": null,
+    "sticky": false
+  }
+]