fixes around publishing behaviour + testing

backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/ApplicationRolloutStrategySqlApi.kt

@@ -19,7 +19,7 @@ import java.util.*
 
 class ApplicationRolloutStrategySqlApi @Inject constructor(
   private val conversions: Conversions,
-  private val archiveStrategy: ArchiveStrategy, private val internalFeatureApi: InternalFeatureApi
+  private val internalFeatureApi: InternalFeatureApi
 ) : ApplicationRolloutStrategyApi {
 
   override fun createStrategy(
@@ -118,6 +118,8 @@ class ApplicationRolloutStrategySqlApi @Inject constructor(
     if (strategy.application.id == app.id) {
       var notifyAttachedFeatures = false
 
+      val originalRolloutStrategy = InternalFeatureApi.toRolloutStrategy(strategy)
+
       // check if we are renaming it and if so, are we using a duplicate name
       update.name?.let { newName ->
         if (!strategy.name.equals(newName, ignoreCase = true)) {
@@ -166,7 +168,7 @@ class ApplicationRolloutStrategySqlApi @Inject constructor(
           // now we have to update all of the tagged features, add an additional history element, force republishing of everything associated
           strategy.sharedRolloutStrategies?.let { strategies ->
             strategies.forEach { strategyForFeatureValue ->
-              internalFeatureApi.updatedApplicationStrategy(strategyForFeatureValue, p)
+              internalFeatureApi.updatedApplicationStrategy(strategyForFeatureValue, originalRolloutStrategy, p)
             }
           }
         }
@@ -253,6 +255,8 @@ class ApplicationRolloutStrategySqlApi @Inject constructor(
     // only update and publish if it _actually_ changed. We do this here instead of in ArchiveStrategy
     // because its not a simple publish. That class normally orchestrates it, but its simply too complex.
     if (strategy.whenArchived == null) {
+      val originalRolloutStrategy = InternalFeatureApi.toRolloutStrategy(strategy)
+
       strategy.whenArchived = LocalDateTime.now()
       strategy.name = (strategy.name + Conversions.archivePrefix + isoDate.format(strategy.whenArchived)).take(150)
       strategy.whoChanged = p
@@ -264,7 +268,7 @@ class ApplicationRolloutStrategySqlApi @Inject constructor(
 
         copy.forEach { strategyForFeatureValue ->
           // this needs to remove the connection, create an audit trail, and publish a new record to Edge, and trigger webhooks
-          internalFeatureApi.detachApplicationStrategy(strategyForFeatureValue, strategy, p)
+          internalFeatureApi.detachApplicationStrategy(strategyForFeatureValue, originalRolloutStrategy, p)
         }
       }
     }

backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/FeatureSqlApi.kt

@@ -36,15 +36,30 @@ interface InternalFeatureApi {
   fun forceVersionBump(featureIds: List<UUID>, envId: UUID)
   fun updatedApplicationStrategy(
     strategyForFeatureValue: DbStrategyForFeatureValue,
+    originalStrategy: RolloutStrategy,
     personWhoUpdated: DbPerson
   )
   fun detachApplicationStrategy(
     strategyForFeatureValue: DbStrategyForFeatureValue,
-    strategy: DbApplicationRolloutStrategy,
+    originalStrategy: RolloutStrategy,
     personWhoArchived: DbPerson
   )
 
-  fun toRolloutStrategy(sharedStrategy: DbStrategyForFeatureValue): RolloutStrategy
+  companion object  {
+    fun toRolloutStrategy(appStrategy: DbApplicationRolloutStrategy): RolloutStrategy {
+      return RolloutStrategy().id(appStrategy.shortUniqueCode)
+        .percentage(appStrategy.strategy.percentage)
+        .percentageAttributes(appStrategy.strategy.percentageAttributes)
+        .attributes(appStrategy.strategy.attributes)
+        .avatar(appStrategy.strategy.avatar)
+        .colouring(appStrategy.strategy.colouring)
+        .name(appStrategy.name).disabled(false)
+    }
+
+    fun toRolloutStrategy(sharedStrategy: DbStrategyForFeatureValue): RolloutStrategy {
+      return toRolloutStrategy(sharedStrategy.rolloutStrategy).value(sharedStrategy.value)
+    }
+  }
 }
 
 class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conversions,
@@ -67,10 +82,6 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver
     }
   }
 
-  override fun toRolloutStrategy(sharedStrategy: DbStrategyForFeatureValue): RolloutStrategy {
-    val appStrategy = sharedStrategy.rolloutStrategy
-    return RolloutStrategy().id(appStrategy.shortUniqueCode).name(appStrategy.name).value(sharedStrategy.value).disabled(false)
-  }
 
   override fun forceVersionBump(featureIds: List<UUID>, envId: UUID) {
     // force a version change on all these features
@@ -84,24 +95,29 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver
 
   override fun updatedApplicationStrategy(
     strategyForFeatureValue: DbStrategyForFeatureValue,
+    originalStrategy: RolloutStrategy,
     personWhoUpdated: DbPerson
   ) {
     // we need to bump the feature value version even though nothing ostensibly changed
     strategyForFeatureValue.featureValue.let { fv ->
-      val priorStrategies = fv.sharedRolloutStrategies.map { toRolloutStrategy(it) }
+      val priorStrategies = fv.sharedRolloutStrategies.map { InternalFeatureApi.toRolloutStrategy(it) }
 
       fv.whoUpdated = personWhoUpdated
       saveFeatureValue(fv, true)
 
       // this will cause an audit webhook automatically
       cacheSource.publishFeatureChange(fv)
 
-      publishFeatureMessage(fv, strategyForFeatureValue, "updated", priorStrategies)
+      publishFeatureMessage(fv, "updated", priorStrategies, originalStrategy,
+        InternalFeatureApi.toRolloutStrategy(strategyForFeatureValue))
     }
   }
 
   private fun publishFeatureMessage(fv: DbFeatureValue,
-                                    ue: DbStrategyForFeatureValue, action: String, priorStrategies: List<RolloutStrategy>) {
+                                    action: String,
+                                    priorStrategies: List<RolloutStrategy>,
+                                    originalStrategy: RolloutStrategy,
+                                    newStrategy: RolloutStrategy?) {
     val singleNotUpdated = SingleFeatureValueUpdate(hasChanged = false, updated = false, previous = false)
 
     try {
@@ -113,7 +129,7 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver
           MultiFeatureValueUpdate(),
           MultiFeatureValueUpdate(
             true,
-            mutableListOf(RolloutStrategyUpdate(type = action, old = toRolloutStrategy(ue))),
+            mutableListOf(RolloutStrategyUpdate(type = action, old = originalStrategy, new = newStrategy)),
             mutableListOf(),
             priorStrategies
           ),
@@ -129,20 +145,24 @@ class InternalFeatureSqlApi @Inject constructor(private val convertUtils: Conver
   // this needs to remove the connection, create an audit trail, and publish a new record to Edge, and trigger webhooks
   override fun detachApplicationStrategy(
     strategyForFeatureValue: DbStrategyForFeatureValue,
-    strategy: DbApplicationRolloutStrategy,
+    originalStrategy: RolloutStrategy,
     personWhoArchived: DbPerson
   ) {
     // this removes the strategy and forces the feature to update & create a new historical record
     QDbFeatureValue().id.eq(strategyForFeatureValue.featureValue.id).findOne()?.let { fv ->
-      val priorStrategies = fv.sharedRolloutStrategies.map { toRolloutStrategy(it) }
+      val priorStrategies = fv.sharedRolloutStrategies.map { InternalFeatureApi.toRolloutStrategy(it) }
+      // hold onto it because the object will be deleted
+      val originalValue = strategyForFeatureValue.value
 
       if (fv.sharedRolloutStrategies.removeIf { it.id == strategyForFeatureValue.id }) {
         fv.whoUpdated = personWhoArchived
         saveFeatureValue(fv, true)
 
         // this will cause an audit webhook automatically
         cacheSource.publishFeatureChange(fv)
-        publishFeatureMessage(fv, strategyForFeatureValue, "deleted", priorStrategies)
+        publishFeatureMessage(fv, "deleted",
+          priorStrategies,
+          originalStrategy.value(originalValue), null)
       }
     }
   }
@@ -338,7 +358,7 @@ class FeatureSqlApi @Inject constructor(
     val applicationStrategyUpdates = MultiFeatureValueUpdate<RolloutStrategyUpdate, RolloutStrategy>()
     applicationStrategyUpdates.hasChanged = existing.sharedRolloutStrategies.isNotEmpty()
     existing.sharedRolloutStrategies.forEach { rsi ->
-      applicationStrategyUpdates.updated.add(RolloutStrategyUpdate(type = "added", new = internalFeatureApi.toRolloutStrategy(rsi)))
+      applicationStrategyUpdates.updated.add(RolloutStrategyUpdate(type = "added", new = InternalFeatureApi.toRolloutStrategy(rsi)))
     }
     val retiredUpdate =
       SingleFeatureValueUpdate(hasChanged = featureValue.retired, updated = featureValue.retired, previous = false)
@@ -603,7 +623,7 @@ class FeatureSqlApi @Inject constructor(
         changed = true
         if (todel) {
           addToStrategyUpdates(type = "deleted",
-            oldStrategy = internalFeatureApi.toRolloutStrategy(strategy),
+            oldStrategy = InternalFeatureApi.toRolloutStrategy(strategy),
             strategyUpdates = strategyUpdates)
         }
         todel
@@ -627,7 +647,7 @@ class FeatureSqlApi @Inject constructor(
         log.debug("trying to reorder strategies and no change value permission")
         throw FeatureApi.NoAppropriateRole()
       }
-      addToStrategyReorders(strategyUpdates, desiredList.map { internalFeatureApi.toRolloutStrategy(it) }.toMutableList(),
+      addToStrategyReorders(strategyUpdates, desiredList.map { InternalFeatureApi.toRolloutStrategy(it) }.toMutableList(),
         historical.map { sharedStrategyToRolloutStrategyForReporting(it, foundApplicationStrategies) })
 
       changed = true

backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategiesSpec.groovy

@@ -1,13 +1,13 @@
 package io.featurehub.db.services
 
-import io.featurehub.dacha.model.PublishAction
+
 import io.featurehub.db.api.Opts
 import io.featurehub.db.api.PersonFeaturePermission
+import io.featurehub.db.messaging.FeatureMessagingParameter
+import io.featurehub.db.messaging.FeatureMessagingPublisher
 import io.featurehub.db.model.DbApplicationRolloutStrategy
 import io.featurehub.db.model.DbFeatureValue
-import io.featurehub.db.model.DbPerson
 import io.featurehub.db.model.DbStrategyForFeatureValue
-import io.featurehub.db.model.RoleType
 import io.featurehub.db.publish.CacheSourceFeatureGroupApi
 import io.featurehub.mr.events.common.CacheSource
 import io.featurehub.mr.model.ApplicationRoleType
@@ -16,6 +16,7 @@ import io.featurehub.mr.model.CreateApplicationRolloutStrategy
 import io.featurehub.mr.model.CreateFeature
 import io.featurehub.mr.model.FeatureValue
 import io.featurehub.mr.model.FeatureValueType
+import io.featurehub.mr.model.RolloutStrategy
 import io.featurehub.mr.model.RolloutStrategyInstance
 import io.featurehub.mr.model.UpdateApplicationRolloutStrategy
 
@@ -26,7 +27,7 @@ class ApplicationStrategiesSpec extends Base3Spec {
     cacheSource = Mock(CacheSource)
     archiveStrategy = Mock(ArchiveStrategy)
     internalFeatureApi = Mock(InternalFeatureApi)
-    applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi)
+    applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi)
     featureSqlApi = new FeatureSqlApi(convertUtils, cacheSource, rsValidator, featureMessagingCloudEventPublisher, Mock(CacheSourceFeatureGroupApi))
   }
 
@@ -65,13 +66,12 @@ class ApplicationStrategiesSpec extends Base3Spec {
   def "full lifecycle of application strategies"() {
     given: "i have a feature for the application"
       String key = createFeature()
-    and: "full permissions"
-      def perms = allPermissions()
     and: "i have an application strategy"
       def strategy = createStrategy()
+
       def updateName = ranName()
     when: "i associate the strategy with the feature value in the default environment"
-      def fv = associateStrategyWithFeatureValue(key, strategy)
+      associateStrategyWithFeatureValue(key, strategy)
     then:
       with(cacheSource) {
         1 * publishFeatureChange( { DbFeatureValue v ->
@@ -81,10 +81,16 @@ class ApplicationStrategiesSpec extends Base3Spec {
       0 * _
     when: "i update the application strategy"
       applicationRolloutStrategySqlApi.updateStrategy(app1.id, strategy.id,
-        new UpdateApplicationRolloutStrategy().name(updateName), superPerson.id.id, Opts.empty())
+        new UpdateApplicationRolloutStrategy().percentage(20).name(updateName), superPerson.id.id, Opts.empty())
     then:
+      1 *
       1 * internalFeatureApi.updatedApplicationStrategy({ DbStrategyForFeatureValue fvStrategy ->
         fvStrategy.rolloutStrategy.strategy.name == updateName
+        fvStrategy.rolloutStrategy.strategy.percentage == 20
+      }, { RolloutStrategy original ->
+        original != null
+        original.percentage == null
+        original.name == strategy.name
       }, _)
   }
 
@@ -97,7 +103,7 @@ class ApplicationStrategiesSpec extends Base3Spec {
       keys.each { associateStrategyWithFeatureValue(it, strategy) }
     and: "we reset the mock"
       internalFeatureApi = Mock(InternalFeatureApi)
-      applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi)
+      applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi)
     when: "we update the strategy name"
       applicationRolloutStrategySqlApi.updateStrategy(app1.id, strategy.id, new UpdateApplicationRolloutStrategy().name(ranName()), superuser, Opts.empty())
     then: "there is no interaction with the features as nothing changed that was important"
@@ -108,18 +114,54 @@ class ApplicationStrategiesSpec extends Base3Spec {
     then:
       2 * internalFeatureApi.updatedApplicationStrategy({ DbStrategyForFeatureValue fvStrategy ->
         keys.contains(fvStrategy.featureValue.feature.key)
-      }, _)
+      }, { RolloutStrategy rs -> rs.percentage == null }, _)
     when:
       applicationRolloutStrategySqlApi.archiveStrategy(app1.id, strategy.id, superuser)
-    then:
+    then: // if we do this in separate then:'s then we can recognize the different calls the same method
       1 * internalFeatureApi.detachApplicationStrategy({ DbStrategyForFeatureValue fv ->
         fv.featureValue.feature.key == keys.first()
-      }, { DbApplicationRolloutStrategy s ->
-        s.name != strategy.name
-      }, _)
+      }, { RolloutStrategy rs -> rs.percentage == 1000 },  _)
     then:
       1 * internalFeatureApi.detachApplicationStrategy({ DbStrategyForFeatureValue fv ->
         fv.featureValue.feature.key == keys.last()
       }, _, _)
   }
+
+  def "when we use the real internal feature service and update and delete strategies"() {
+    given: "we create two features"
+      def keys = [createFeature()]
+    and: "we create one strategy"
+      def strategy = createStrategy()
+    and: "we associate the strategy with both features"
+      keys.each { associateStrategyWithFeatureValue(it, strategy) }
+    and: "we ensure we are not using a mock internal feature service"
+      FeatureMessagingPublisher fmp = Mock()
+      internalFeatureApi = new InternalFeatureSqlApi(convertUtils, cacheSource, fmp)
+      applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi)
+    when: "we update the strategy percentage"
+      applicationRolloutStrategySqlApi.updateStrategy(app1.id, strategy.id,
+        new UpdateApplicationRolloutStrategy().percentage(1000), superuser, Opts.empty())
+    then:
+      1 * cacheSource.publishFeatureChange({ DbFeatureValue fv ->
+        fv.feature.key == keys.first()
+        fv.version == 3
+      })
+      1 * fmp.publish({ FeatureMessagingParameter p ->
+        p.applicationStrategyUpdates.hasChanged
+        p.applicationStrategyUpdates.updated[0].new.percentage == 1000
+        p.applicationStrategyUpdates.updated[0].old.percentage == null
+      }, _)
+    when: "we delete the application strategy"
+      applicationRolloutStrategySqlApi.archiveStrategy(app1.id, strategy.id, superuser)
+    then:
+      1 * cacheSource.publishFeatureChange({ DbFeatureValue fv ->
+        fv.feature.key == keys.first()
+        fv.version == 4
+      })
+      1 * fmp.publish({ FeatureMessagingParameter p ->
+        p.applicationStrategyUpdates.hasChanged
+        p.applicationStrategyUpdates.updated[0].new == null
+        p.applicationStrategyUpdates.updated[0].old.percentage == 1000
+      }, _)
+  }
 }

backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ApplicationStrategySpec.groovy

@@ -13,7 +13,7 @@ class ApplicationStrategySpec extends Base3Spec {
   ApplicationRolloutStrategySqlApi appStrategyApi
 
   def setup() {
-    appStrategyApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi)
+    appStrategyApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi)
   }
 
   def "i can create, update and delete an application strategy"() {

backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/FeatureAuditingApplicationStrategiesSpec.groovy

@@ -40,7 +40,7 @@ class FeatureAuditingApplicationStrategiesSpec extends Base3Spec {
   DbFeatureValueVersionKey histId
 
   def setup() {
-    applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, archiveStrategy, internalFeatureApi)
+    applicationRolloutStrategySqlApi = new ApplicationRolloutStrategySqlApi(convertUtils, internalFeatureApi)
 
     dbEnvironment = findEnvironment(env1.id)
     dbApplication = findApplication(app1.id)