Repository: RasaHQ/rasa
Pull request: ATO 207 run form validation  on form activation
**Proposed changes**:
- Revert change here https://github.com/RasaHQ/rasa/pull/10295/files#r807244104 that removed running the form validation action on activation of the form (before the loop is active).
- This would make #10913 obsolete since the 2.x mechanism for achieving the same thing would be returned.
- Closes #11199 
- Resolves the issue where any `extract_<slot_name>` method in a FormValidationAction subclass only runs after a required_slot in the form has been set (i.e. after activation).

**Status (please check what you already did)**:
- [x] added some tests for the functionality
- [ ] updated the documentation
- [x] 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:
required_slots function in form is not called
h3\. Rasa Open Source version

3.1.0

h3\. Rasa SDK version

3.1.2

h3\. Python version

3.7

h3\. What operating system are you using?

Windows

h3\. What happened?

I've created a form with form validation. Form doesn't call my required_slots method in spite of I've created a custom required_slots function. This is my code:

It calls firstly action_ask_totalizator_block_form_card_number action. But I expect firstly to go to my overriden required_slots function. (I tested on other forms and this works well)

    class ValidateTotalizatorBlockForm(FormValidationAction):

        def name(self) -> Text:
            """Unique identifier of the form. """
            return "validate_totalizator_block_form"

        async def required_slots(
                self,
                domain_slots: List[Text],
                dispatcher: "CollectingDispatcher",
                tracker: "Tracker",
                domain: "DomainDict",
        ) -> Optional[List[Text]]:
            token = get_metadata_parameters(tracker, key="token")
            if not token:
                return []

            number = tracker.get_slot("number")
            card_number = tracker.get_slot("card_number")
            otp = tracker.get_slot("otp")
            accept = tracker.get_slot("accept")

            logger.info(f"number {number}")
            logger.info(f"card_number {card_number}")
            logger.info(f"otp {otp}")
            logger.info(f"accept {accept}")

            if card_number and accept == "no":
                return []

            return ["card_number", "accept", "otp"]

    domain.yml
      - action_ask_totalizator_block_form_card_number
      - action_ask_totalizator_block_form_otp
      - action_submit_totalizator_block_form
      - validate_totalizator_block_form

      totalizator_block_form:
        required_slots:
          - card_number
          - accept
          - otp

    rules:
    - rule: Activate totalizator_block_form
      steps:
      - intent: totalizator.block
      - action: totalizator_block_form
      - active_loop: totalizator_block_form # Keep requesting form slots until they're filled in

    - rule: Submit totalizator_block_form
      condition: # Condition that form is active
      - active_loop: totalizator_block_form
      steps:
      - action: totalizator_block_form
      - active_loop: null # Stop the form
      - slot_was_set:
        - requested_slot: null
      - action: action_submit_totalizator_block_form

h3\. Command / Request

When I trigger `totalizator.block` intent, it goes to my `action_ask_totalizator_block_form_card_number`. I want it to go to required_slots function firstly.

h3\. Relevant log output

<pre>Traceback (most recent call last):
  File "handle_request", line 83, in handle_request
    )
  File "C:\Users\igeladze\AppData\Local\Continuum\anaconda3\envs\rasa3\lib\site-packages\rasa_sdk\endpoint.py", line 104, in webhook
    result = await executor.run(action_call)
  File "C:\Users\igeladze\AppData\Local\Continuum\anaconda3\envs\rasa3\lib\site-packages\rasa_sdk\executor.py", line 398, in run
    action(dispatcher, tracker, domain)
  File "C:\Users\igeladze\AppData\Local\Continuum\anaconda3\envs\rasa3\lib\site-packages\rasa_sdk\utils.py", line 230, in call_potential_coroutine
    return await coroutine_or_return_value
  File "C:\Users\igeladze\Desktop\BOG_Projects\Chatbot3\actions\form\totalizator_block_form.py", line 287, in run
    message, buttons, card_number_and_card_id_map = self.get_card_list(tracker)
  File "C:\Users\igeladze\Desktop\BOG_Projects\Chatbot3\actions\form\totalizator_block_form.py", line 239, in get_card_list
    channel = chat_rb_channel_mapping(channel)
  File "C:\Users\igeladze\Desktop\BOG_Projects\Chatbot3\common\general.py", line 65, in chat_rb_channel_mapping
    assert chat_channel in channels_mapping</pre>

This happens because it calls action_ask_totalizator_block_form_card_number function and not my required_slots function. In my required_slots function, I return [] if user is not logged in. Then it should go to submit and this error will not happen. My question is why it doesnt go to my required_slots function firstly.

Hints from issue discussion:
I have encountered this as well, when migrating from Rasa 2 to Rasa 3.

In my experience, Rasa 2 called this function before requesting the first slot.
Now, Rasa 3 calls the function after the first requested slot is attemped to be filled.

A **workaround** I have used to mimic Rasa 2 behaviour is to list a dummy pre-filled slot as the first slot in the forms `.yml` defintion. Since it's already filled it won't be requested again.
This [PR](https://github.com/RasaHQ/rasa/pull/11149) which was part of `3.1.1` addresses a related issue about extracting slots that have `from_trigger_intent` mappings. @jonahgeladze1 I can't run your example as-is since it's not quite complete, could you please confirm if the issue persists on `3.1.1` as well?
@melindaloubser1 I've just tested it on 3.1.1 and 3.2.1 version, but it doesn't work anyways.
@ScottWalkerAU I've tested your solution and it works well, but seems really artificial. 
I think that form should firstly check the required_slots function and after that any "utter_ask" functions.
I created this minimal reproducible example here to validate this bug (nothing new to those on this issue, just for future reference)
 https://github.com/melindaloubser1/moodbot/tree/bug_required_slots_not_called

Commit message: match mocking methods in other tests

Patch to review:
diff --git a/changelog/11326.bugfix.md b/changelog/11326.bugfix.md
new file mode 100644
index 000000000000..ee01ea17b258
--- /dev/null
+++ b/changelog/11326.bugfix.md
@@ -0,0 +1 @@
+Revert change in #10295 that removed running the form validation action on activation of the form before the loop is active.
\ No newline at end of file
diff --git a/rasa/core/actions/forms.py b/rasa/core/actions/forms.py
index c2a30ef69971..0e5106a53cdf 100644
--- a/rasa/core/actions/forms.py
+++ b/rasa/core/actions/forms.py
@@ -534,11 +534,8 @@ async def _validate_if_required(
            - the form is called after `action_listen`
            - form validation was not cancelled
         """
-        # No active_loop means there are no form filled slots to validate yet
-        if not tracker.active_loop:
-            return []
-
-        needs_validation = (
+        # no active_loop means that it is called during activation
+        needs_validation = not tracker.active_loop or (
             tracker.latest_action_name == ACTION_LISTEN_NAME
             and not tracker.is_active_loop_interrupted
         )
diff --git a/tests/core/actions/test_forms.py b/tests/core/actions/test_forms.py
index a353f4c7eba7..55b4b9640c82 100644
--- a/tests/core/actions/test_forms.py
+++ b/tests/core/actions/test_forms.py
@@ -5,6 +5,7 @@
 import pytest
 from _pytest.monkeypatch import MonkeyPatch
 from aioresponses import aioresponses
+from questionary import form
 
 from rasa.core.agent import Agent
 from rasa.core.policies.policy import PolicyPrediction
@@ -34,38 +35,69 @@
 from rasa.shared.core.trackers import DialogueStateTracker
 from rasa.utils.endpoints import EndpointConfig
 
+ACTION_SERVER_URL = "http://my-action-server:5055/webhook"
+
 
 async def test_activate():
     tracker = DialogueStateTracker.from_events(sender_id="bla", evts=[])
-    form_name = "my form"
-    action = FormAction(form_name, None)
-    slot_name = "num_people"
+    form_name = "my_form"
+    action_server = EndpointConfig(ACTION_SERVER_URL)
+    action = FormAction(form_name, action_server)
+    domain_required_slot_name = "num_people"
+    slot_set_by_remote_custom_extraction_method = "some_slot"
+    slot_value_set_by_remote_custom_extraction_method = "anything"
     domain = textwrap.dedent(
         f"""
     slots:
-      {slot_name}:
+      {domain_required_slot_name}:
         type: float
         mappings:
         - type: from_entity
           entity: number
+      {slot_set_by_remote_custom_extraction_method}:
+          type: any
+          mappings:
+          - type: custom
     forms:
       {form_name}:
         {REQUIRED_SLOTS_KEY}:
-        - {slot_name}
+        - {domain_required_slot_name}
     responses:
       utter_ask_num_people:
       - text: "How many people?"
+    actions:
+      - validate_{form_name}
       """
     )
     domain = Domain.from_yaml(domain)
 
-    events = await action.run(
-        CollectingOutputChannel(),
-        TemplatedNaturalLanguageGenerator(domain.responses),
-        tracker,
-        domain,
-    )
-    assert events[:-1] == [ActiveLoop(form_name), SlotSet(REQUESTED_SLOT, slot_name)]
+    form_validation_events = [
+        {
+            "event": "slot",
+            "timestamp": None,
+            "name": slot_set_by_remote_custom_extraction_method,
+            "value": slot_value_set_by_remote_custom_extraction_method,
+        }
+    ]
+    with aioresponses() as mocked:
+        mocked.post(
+            ACTION_SERVER_URL,
+            payload={"events": form_validation_events},
+        )
+        events = await action.run(
+            CollectingOutputChannel(),
+            TemplatedNaturalLanguageGenerator(domain.responses),
+            tracker,
+            domain,
+        )
+    assert events[:-1] == [
+        ActiveLoop(form_name),
+        SlotSet(
+            slot_set_by_remote_custom_extraction_method,
+            slot_value_set_by_remote_custom_extraction_method,
+        ),
+        SlotSet(REQUESTED_SLOT, domain_required_slot_name),
+    ]
     assert isinstance(events[-1], BotUttered)
 
 
@@ -575,12 +607,10 @@ async def test_validate_slots(
     assert slot_events == [SlotSet(slot_name, slot_value), SlotSet("num_tables", 5)]
     tracker.update_with_events(slot_events, domain)
 
-    action_server_url = "http:/my-action-server:5055/webhook"
-
     with aioresponses() as mocked:
-        mocked.post(action_server_url, payload={"events": validate_return_events})
+        mocked.post(ACTION_SERVER_URL, payload={"events": validate_return_events})
 
-        action_server = EndpointConfig(action_server_url)
+        action_server = EndpointConfig(ACTION_SERVER_URL)
         action = FormAction(form_name, action_server)
 
         events = await action.run(
@@ -633,8 +663,6 @@ async def test_request_correct_slots_after_unhappy_path_with_custom_required_slo
         ],
     )
 
-    action_server_url = "http://my-action-server:5055/webhook"
-
     # Custom form validation action changes the order of the requested slots
     validate_return_events = [
         {"event": "slot", "name": REQUESTED_SLOT, "value": slot_name_2}
@@ -644,9 +672,9 @@ async def test_request_correct_slots_after_unhappy_path_with_custom_required_slo
     expected_events = [SlotSet(REQUESTED_SLOT, slot_name_2)]
 
     with aioresponses() as mocked:
-        mocked.post(action_server_url, payload={"events": validate_return_events})
+        mocked.post(ACTION_SERVER_URL, payload={"events": validate_return_events})
 
-        action_server = EndpointConfig(action_server_url)
+        action_server = EndpointConfig(ACTION_SERVER_URL)
         action = FormAction(form_name, action_server)
 
         events = await action.run(
@@ -694,12 +722,11 @@ async def test_no_slots_extracted_with_custom_slot_mappings(custom_events: List[
     - validate_{form_name}
     """
     domain = Domain.from_yaml(domain)
-    action_server_url = "http:/my-action-server:5055/webhook"
 
     with aioresponses() as mocked:
-        mocked.post(action_server_url, payload={"events": custom_events})
+        mocked.post(ACTION_SERVER_URL, payload={"events": custom_events})
 
-        action_server = EndpointConfig(action_server_url)
+        action_server = EndpointConfig(ACTION_SERVER_URL)
         action = FormAction(form_name, action_server)
 
         with pytest.raises(ActionExecutionRejection):
@@ -736,12 +763,10 @@ async def test_validate_slots_on_activation_with_other_action_after_user_utteran
     - validate_{form_name}
     """
     domain = Domain.from_yaml(domain)
-    action_server_url = "http:/my-action-server:5055/webhook"
-
     expected_slot_value = "✅"
     with aioresponses() as mocked:
         mocked.post(
-            action_server_url,
+            ACTION_SERVER_URL,
             payload={
                 "events": [
                     {"event": "slot", "name": slot_name, "value": expected_slot_value}
@@ -749,7 +774,7 @@ async def test_validate_slots_on_activation_with_other_action_after_user_utteran
             },
         )
 
-        action_server = EndpointConfig(action_server_url)
+        action_server = EndpointConfig(ACTION_SERVER_URL)
         action_extract_slots = ActionExtractSlots(action_endpoint=None)
         slot_events = await action_extract_slots.run(
             CollectingOutputChannel(),
@@ -761,6 +786,10 @@ async def test_validate_slots_on_activation_with_other_action_after_user_utteran
 
         form_action = FormAction(form_name, action_server)
 
+        mocked.post(
+            ACTION_SERVER_URL,
+            payload={"events": []},
+        )
         events = await form_action.run(
             CollectingOutputChannel(),
             TemplatedNaturalLanguageGenerator(domain.responses),
@@ -1663,17 +1692,15 @@ async def test_action_extract_slots_custom_mapping_with_condition():
         sender_id="test_id", evts=events, slots=domain.slots
     )
 
-    action_server_url = "http:/my-action-server:5055/webhook"
-
     with aioresponses() as mocked:
         mocked.post(
-            action_server_url,
+            ACTION_SERVER_URL,
             payload={
                 "events": [{"event": "slot", "name": "custom_slot", "value": "test"}]
             },
         )
 
-        action_server = EndpointConfig(action_server_url)
+        action_server = EndpointConfig(ACTION_SERVER_URL)
         action_extract_slots = ActionExtractSlots(action_server)
         events = await action_extract_slots.run(
             CollectingOutputChannel(),


Code review comments:
diff_hunk: @@ -5,6 +5,7 @@
 import pytest
 from _pytest.monkeypatch import MonkeyPatch
 from aioresponses import aioresponses
+from questionary import form
path: tests/core/actions/test_forms.py
text: @user1:
Why do you need this import, it seems to cause a lot of code quality fails?

@author:
entirely accidental, probably an auto import by my IDE