• 
      

    Fix manual Status Update runs with multiple candidate integration configs.

    Review Request #12835 — Created Feb. 21, 2023 and submitted

    Information

    Review Board
    release-5.0.x

    Reviewers

    When performing a manual run of a StatusUpdate, the code responding to
    the manual run signal had no choice but to try to find a matching
    IntegrationConfig for the review request. If multiple matches were
    found, it had to pick one, and this could easily be the wrong one.

    To solve this, we now store the configuration ID in
    StatusUpdate.extra_data, using a private key. This is stored either
    when setting the new StatusUpdate.integration_config property, or when
    creating the StatusUpdate using
    StatusUpdate.objects.create_for_integration() (which is the preferred
    way going forward).

    When either setting or fetching the configuration, the configuration is
    validated to make sure it matches the same LocalSite, to avoid any
    cross-site contamination issues.

    The status_update_request_run signal is now given a config argument
    mapping to this stored configuration (if one is stored), which
    integrations can use. This will require a change for integrations
    handling that signal to prioritize the new argument.

    Old Status Updates in manual run mode will result in a None
    configuration, which will need to be handled through legacy means, but
    all future Status Updates created after this change will have a suitable
    configuration.

    All unit tests pass.

    Tested this along with changes in rbintegrations to make use of the
    new config state.

    This will be tested in production with an affected customer.

    Summary ID
    Fix manual Status Update runs with multiple candidate integration configs.
    When performing a manual run of a `StatusUpdate`, the code responding to the manual run signal had no choice but to try to find a matching `IntegrationConfig` for the review request. If multiple matches were found, it had to pick one, and this could easily be the wrong one. To solve this, we now store the configuration ID in `StatusUpdate.extra_data`, using a private key. This is stored either when setting the new `StatusUpdate.integration_config` property, or when creating the `StatusUpdate` using `StatusUpdate.objects.create_for_integration()` (which is the preferred way going forward). When either setting or fetching the configuration, the configuration is validated to make sure it matches the same `LocalSite`, to avoid any cross-site contamination issues. The `status_update_request_run` signal is now given a `config` argument mapping to this stored configuration (if one is stored), which integrations can use. This will require a change for integrations handling that signal to prioritize the new argument. Old Status Updates in manual run mode will result in a `None` configuration, which will need to be handled through legacy means, but all future Status Updates created after this change will have a suitable configuration.
    0e6fcb14583551c29b9a546448c6c8b678369200
    Description From Last Updated

    'typing.Type' imported but unused Column: 1 Error code: F401

    reviewbotreviewbot

    'typing.List' imported but unused Column: 1 Error code: F401

    reviewbotreviewbot

    Single quotes?

    daviddavid
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    chipx86
    david
    1. 
        
    2. reviewboard/reviews/models/status_update.py (Diff revision 2)
       
       
       
      Show all issues

      Single quotes?

    3. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-5.0.x (21e2147)