Repository: RasaHQ/rasa
Pull request: Remove `Domain.slots_for_entities` usage in `DialogueStateTracker.update()`
**Proposed changes**:
- Closes #9744 

**Status (please check what you already did)**:
- [x] added some tests for the functionality
- [ ] updated the documentation
- [ ] updated the changelog (please check [changelog](https://github.com/RasaHQ/rasa/tree/main/changelog) for instructions)
- [X] reformat files using `black` (please check [Readme](https://github.com/RasaHQ/rasa#code-style) for instructions)


Problem statement:
3.0 slot mappings: revisit `Domain.slots_for_entities()` method and its usage
**Overview of the Problem**:
- In rasa 2.0, `Domain.slots_for_entities()` method used to return a list of `SlotSet` events for slots with the same entity name which had auto-fill enabled. 
- In 3.0, auto-fill has been removed and the method was adapted slightly to set any slots if matching entities are extracted. 
- This method is being used in 2 places: `DialogueStateTracker.update()` and `StoryStep.explicit_events()`:

1. `DialogueStateTracker.update()` has 74 usages in the codebase (including tests) that might need to be checked/adapted if this method is removed. Alternatively, if the method should be maintained, the implementation of `DialogueStateTracker.update()` should be checked if we want a certain section concerning `UserUttered` events to be kept.
This might be a duplicate of what the default `action_extract_slots` now does.

- A side effect of this method is that although it generally allows for slots with `FROM_ENTITY` mappings to be updated throughout the conversation, it is not owed to running the `action_extract_slots` default action, and therefore confusingly not logged.

**Definition of Done**:
- [x] investigate the usage ramifications of this method to determine if it makes sense to remove or adapt/ refactor now that the default action_extract_slots runs after every user turn
- [x] investigate adaptation of `DialogueStateTracker.update()` and complete if feasible
- [x] create new issue for deprecating the usage of entities in stories (in `StoryStep.explicit_events`) and carrying exploratory work that the replacement story format with `slot_was_set` maintains consistent behaviour
- [x] handle `EntitiesAdded` case: If an action can also add entities, then we also need to run the `ActionExtractSlots` after these actions
- [x] adapt any unit tests

Hints from issue discussion:

Commit message: fix for testing e2e stories of bot with slots

Patch to review:
diff --git a/data/test_e2ebot/domain.yml b/data/test_e2ebot/domain.yml
index 0208aeb1354e..87b1f8590470 100644
--- a/data/test_e2ebot/domain.yml
+++ b/data/test_e2ebot/domain.yml
@@ -8,6 +8,20 @@ entities:
  - name
  - mood
 
+slots:
+ name:
+  type: text
+  influence_conversation: false
+  mappings:
+   - type: from_entity
+     entity: name
+ mood:
+  type: text
+  influence_conversation: false
+  mappings:
+  - type: from_entity
+    entity: mood
+
 responses:
   utter_greet:
   - text: "hi!"
diff --git a/rasa/core/processor.py b/rasa/core/processor.py
index 72b5d270f2fa..f0aa91a0085b 100644
--- a/rasa/core/processor.py
+++ b/rasa/core/processor.py
@@ -131,7 +131,7 @@ async def handle_message(
             )
             return None
 
-        tracker = await self._run_action_extract_slots(message, tracker)
+        tracker = await self._run_action_extract_slots(message.output_channel, tracker)
 
         await self._run_prediction_loop(message.output_channel, tracker)
 
@@ -143,13 +143,13 @@ async def handle_message(
         return None
 
     async def _run_action_extract_slots(
-        self, message: UserMessage, tracker: DialogueStateTracker,
+        self, output_channel: OutputChannel, tracker: DialogueStateTracker,
     ) -> DialogueStateTracker:
         action_extract_slots = rasa.core.actions.action.action_for_name_or_text(
             ACTION_EXTRACT_SLOTS, self.domain, self.action_endpoint,
         )
         extraction_events = await action_extract_slots.run(
-            message.output_channel, self.nlg, tracker, self.domain
+            output_channel, self.nlg, tracker, self.domain
         )
         tracker.update_with_events(extraction_events, self.domain)
 
@@ -546,6 +546,9 @@ async def trigger_external_user_uttered(
             UserUttered.create_external(intent_name, entity_list, input_channel),
             self.domain,
         )
+
+        tracker = await self._run_action_extract_slots(output_channel, tracker)
+
         await self._run_prediction_loop(output_channel, tracker)
         # save tracker state to continue conversation from this state
         self._save_tracker(tracker)
@@ -742,6 +745,13 @@ async def _run_prediction_loop(
                     self.on_circuit_break(tracker, output_channel, self.nlg)
                 break
 
+            if prediction.is_end_to_end_prediction:
+                logger.debug(
+                    f"An end-to-end prediction was made which has triggered the 2nd "
+                    f"execution of the default action '{ACTION_EXTRACT_SLOTS}'."
+                )
+                tracker = await self._run_action_extract_slots(output_channel, tracker)
+
             should_predict_another_action = await self._run_action(
                 action, tracker, output_channel, self.nlg, prediction
             )
diff --git a/rasa/core/test.py b/rasa/core/test.py
index a7cd12bbfe2f..9e967944bef2 100644
--- a/rasa/core/test.py
+++ b/rasa/core/test.py
@@ -53,7 +53,7 @@
     ENTITY_ATTRIBUTE_TEXT,
 )
 from rasa.constants import RESULTS_FILE, PERCENTAGE_KEY
-from rasa.shared.core.events import ActionExecuted, EntitiesAdded, UserUttered
+from rasa.shared.core.events import ActionExecuted, EntitiesAdded, UserUttered, SlotSet
 from rasa.shared.core.trackers import DialogueStateTracker
 from rasa.shared.nlu.training_data.formats.readerwriter import TrainingDataWriter
 from rasa.shared.importers.importer import TrainingDataImporter
@@ -601,6 +601,12 @@ async def _get_e2e_entity_evaluation_result(
     prediction: PolicyPrediction,
 ) -> Optional[EntityEvaluationResult]:
     previous_event = tracker.events[-1]
+
+    if isinstance(previous_event, SlotSet):
+        # UserUttered events with entities can be followed by SlotSet events
+        # if slots are defined in the domain
+        previous_event = tracker.get_last_event_for(UserUttered)
+
     if isinstance(previous_event, UserUttered):
         entities_predicted_by_policies = [
             entity
diff --git a/rasa/shared/core/trackers.py b/rasa/shared/core/trackers.py
index c8fbec2e208c..969f3b974157 100644
--- a/rasa/shared/core/trackers.py
+++ b/rasa/shared/core/trackers.py
@@ -58,7 +58,6 @@
     ActiveLoop,
     SessionStarted,
     ActionExecutionRejected,
-    EntitiesAdded,
     DefinePrevUserUtteredFeaturization,
 )
 from rasa.shared.core.domain import Domain, State
@@ -651,19 +650,6 @@ def update(self, event: Event, domain: Optional[Domain] = None) -> None:
         self.events.append(event)
         event.apply_to(self)
 
-        if domain and isinstance(event, (UserUttered, EntitiesAdded)):
-            if isinstance(event, UserUttered):
-                # Rather get entities from `parse_data` as
-                # `DefinePrevUserUtteredEntities` might have already affected the
-                # `UserUttered.entities` attribute (this might e.g. happen when the
-                # `InMemoryTrackerStore` is used).
-                entities = event.parse_data[ENTITIES]
-            else:
-                entities = event.entities
-
-            for e in domain.slots_for_entities(entities):
-                self.update(e)
-
     def update_with_events(
         self,
         new_events: List[Event],
diff --git a/tests/core/test_actions.py b/tests/core/test_actions.py
index 39a5c1ea9de1..65dabd96d179 100644
--- a/tests/core/test_actions.py
+++ b/tests/core/test_actions.py
@@ -1260,12 +1260,7 @@ async def test_action_extract_slots_predefined_mappings(
         domain,
     )
 
-    if new_user.entities:
-        # the slot gets filled with extracted entities during tracker.update
-        assert not updated_evts
-        assert tracker.get_slot(slot_name) == updated_value
-    else:
-        assert updated_evts == [SlotSet(slot_name, updated_value)]
+    assert updated_evts == [SlotSet(slot_name, updated_value)]
 
 
 async def test_action_extract_slots_with_from_trigger_mappings():
diff --git a/tests/core/test_processor.py b/tests/core/test_processor.py
index 9f0f18ec9a2a..1e5547ffdde8 100644
--- a/tests/core/test_processor.py
+++ b/tests/core/test_processor.py
@@ -1475,3 +1475,18 @@ def test_get_tracker_adds_model_id(default_processor: MessageProcessor):
     model_id = default_processor.model_metadata.model_id
     tracker = default_processor.get_tracker("bloop")
     assert tracker.model_id == model_id
+
+
+async def test_processor_e2e_slot_set(e2e_bot_agent: Agent, caplog: LogCaptureFixture):
+    processor = e2e_bot_agent.processor
+    message = UserMessage("I am feeling sad.", CollectingOutputChannel(), "test",)
+    with caplog.at_level(logging.DEBUG):
+        await processor.handle_message(message)
+
+    tracker = processor.get_tracker("test")
+    assert SlotSet("mood", "sad") in tracker.events
+    assert any(
+        "An end-to-end prediction was made which has triggered the 2nd execution of "
+        "the default action 'action_extract_slots'." in message
+        for message in caplog.messages
+    )
diff --git a/tests/shared/core/test_trackers.py b/tests/shared/core/test_trackers.py
index 7d2f98e0c8ce..0a8bde4af524 100644
--- a/tests/shared/core/test_trackers.py
+++ b/tests/shared/core/test_trackers.py
@@ -11,6 +11,9 @@
 import freezegun
 import pytest
 
+from rasa.core.actions.action import ActionExtractSlots
+from rasa.core.channels import CollectingOutputChannel
+from rasa.core.nlg import TemplatedNaturalLanguageGenerator
 from rasa.core.training import load_data
 import rasa.shared.utils.io
 import rasa.utils.io
@@ -317,7 +320,7 @@ def test_get_latest_entity_values(
     assert list(tracker.get_latest_entity_values("unknown")) == []
 
 
-def test_tracker_update_slots_with_entity(domain: Domain):
+async def test_tracker_update_slots_with_entity(domain: Domain):
     tracker = DialogueStateTracker("default", domain.slots)
 
     test_entity = domain.entities[0]
@@ -340,6 +343,15 @@ def test_tracker_update_slots_with_entity(domain: Domain):
         ),
         domain,
     )
+    action_extract_slots = ActionExtractSlots(action_endpoint=None)
+
+    events = await action_extract_slots.run(
+        CollectingOutputChannel(),
+        TemplatedNaturalLanguageGenerator(domain.responses),
+        tracker,
+        domain,
+    )
+    tracker.update_with_events(events, domain)
 
     assert tracker.get_slot(test_entity) == expected_slot_value
 
@@ -1364,7 +1376,7 @@ def test_policy_prediction_reflected_in_tracker_state():
     assert tracker.latest_message.parse_data["entities"] == nlu_entities
 
 
-def test_autofill_slots_for_policy_entities():
+async def test_fill_slots_for_policy_entities():
     policy_entity, policy_entity_value = "policy_entity", "end-to-end"
     nlu_entity, nlu_entity_value = "nlu_entity", "nlu rocks"
     domain = Domain.from_yaml(
@@ -1410,10 +1422,6 @@ def test_autofill_slots_for_policy_entities():
         slots=domain.slots,
     )
 
-    # Slots are correctly set
-    assert tracker.slots[nlu_entity].value == nlu_entity_value
-    assert tracker.slots[policy_entity].value == policy_entity_value
-
     expected_events = [
         ActionExecuted(ACTION_LISTEN_NAME),
         UserUttered(
@@ -1425,8 +1433,6 @@ def test_autofill_slots_for_policy_entities():
                 {"entity": policy_entity, "value": policy_entity_value},
             ],
         ),
-        # SlotSet event added for entity predicted by NLU
-        SlotSet(nlu_entity, nlu_entity_value),
         DefinePrevUserUtteredFeaturization(True),
         EntitiesAdded(
             entities=[
@@ -1434,13 +1440,24 @@ def test_autofill_slots_for_policy_entities():
                 {"entity": nlu_entity, "value": nlu_entity_value},
             ]
         ),
-        # SlotSet event added for entity predicted by policies
-        # This event is somewhat duplicate. We don't deduplicate as this is a true
-        # reflection of the given events and it doesn't change the actual state.
         SlotSet(nlu_entity, nlu_entity_value),
         SlotSet(policy_entity, policy_entity_value),
     ]
 
+    action_extract_slots = ActionExtractSlots(action_endpoint=None)
+
+    events = await action_extract_slots.run(
+        CollectingOutputChannel(),
+        TemplatedNaturalLanguageGenerator(domain.responses),
+        tracker,
+        domain,
+    )
+    tracker.update_with_events(events, domain)
+
+    # Slots are correctly set
+    assert tracker.slots[nlu_entity].value == nlu_entity_value
+    assert tracker.slots[policy_entity].value == policy_entity_value
+
     for actual, expected in zip(tracker.events, expected_events):
         assert actual == expected
 
diff --git a/tests/shared/core/training_data/story_reader/test_yaml_story_reader.py b/tests/shared/core/training_data/story_reader/test_yaml_story_reader.py
index a84040c1a637..536b4020672d 100644
--- a/tests/shared/core/training_data/story_reader/test_yaml_story_reader.py
+++ b/tests/shared/core/training_data/story_reader/test_yaml_story_reader.py
@@ -173,7 +173,7 @@ async def test_default_slot_value_if_unfeaturized_slot():
     assert not warnings
 
 
-def test_can_read_test_story_with_entities_slot_autofill(domain: Domain):
+def test_can_read_test_story_with_entities(domain: Domain):
     trackers = training.load_data(
         "data/test_yaml_stories/story_with_or_and_entities.yml",
         domain,


Code review comments:
diff_hunk: @@ -601,6 +601,12 @@ async def _get_e2e_entity_evaluation_result(
     prediction: PolicyPrediction,
 ) -> Optional[EntityEvaluationResult]:
     previous_event = tracker.events[-1]
+
+    if isinstance(previous_event, SlotSet):
+        # UserUttered events with entities can be followed by SlotSet events
+        # if slots are defined in the domain
+        previous_event = tracker.get_last_event_for(UserUttered)
path: rasa/core/test.py
text: @user1:
If there was an ActionExecuted in between then this condition should be wrong (only if there was nothing between user uttered and the current action except slots and bot uttered stuff)
```suggestion
    if isinstance(previous_event, SlotSet):
        # UserUttered events with entities can be followed by SlotSet events
        # if slots are defined in the domain
        previous_event = tracker.get_last_event_for((UserUttered, ActionExecuted))
        if isinstance(previous_event, UserUttered):
             ...
```