From 182f18de2ce0f3e01fb7ab46483d499b045c5e80 Mon Sep 17 00:00:00 2001
From: Joey Orlando <joey.orlando@grafana.com>
Date: Mon, 10 Jul 2023 10:59:15 +0200
Subject: [PATCH] fix parsing of grafana feature flags that're enabled via the
 feature_toggles.enabled syntax (#2477)

# What this PR does

Grafana provides two methods for enabling feature flags:
- `feature_toggles.enabled`
- `feature_toggles.<name_of_feature_flag>`

For example, to enable `accessControlOnCall` you could either do:
```json
{
  "feature_toggles": {
    "enabled": "accessControlOnCall,someOtherCoolFeatureFlag"
  }
}
```

or:
```json
{
  "feature_toggles": {
    "accessControlOnCall": true,
    "someOtherCoolFeatureFlag": true
  }
}
```

In method 1, if they're multiple feature flags present, they are _comma
separated, not space separated_. This PR fixes this parsing issue.

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required) (N/A)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
---
 CHANGELOG.md                                         |  7 ++++++-
 engine/apps/grafana_plugin/helpers/client.py         |  4 ++--
 .../grafana_plugin/tests/test_gcom_api_client.py     | 12 +++++++-----
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4db19143..11fa2be9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,7 +16,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Modified DRF pagination class used by `GET /api/internal/v1/alert_receive_channels` and `GET /api/internal/v1/schedules`
   endpoints so that the `next` and `previous` pagination links are properly set when OnCall is run behind
-  a reverse proxy by @joeyorlando ([#TBD](https://github.com/grafana/oncall/pull/TBD))
+  a reverse proxy by @joeyorlando ([#2467](https://github.com/grafana/oncall/pull/2467))
+
+### Fixed
+
+- Address issue where we were improperly parsing Grafana feature flags that were enabled via the `feature_flags.enabled`
+  method by @joeyorlando ([#2477](https://github.com/grafana/oncall/pull/2477))
 
 ## v1.3.7 (2023-07-06)
 
diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py
index c02cddbc..861cba89 100644
--- a/engine/apps/grafana_plugin/helpers/client.py
+++ b/engine/apps/grafana_plugin/helpers/client.py
@@ -284,8 +284,8 @@ class GcomAPIClient(APIClient):
         if not instance_feature_toggles:
             return False
 
-        # features enabled via enable key are space separated
-        features_enabled_via_enable_key = instance_feature_toggles.get("enable", "").split(" ")
+        # features enabled via enable key are comma separated (https://github.com/grafana/grafana/issues/36511)
+        features_enabled_via_enable_key = instance_feature_toggles.get("enable", "").split(",")
         feature_enabled_via_enable_key = feature_name in features_enabled_via_enable_key
         feature_enabled_via_direct_key = instance_feature_toggles.get(feature_name, "false") == "true"
 
diff --git a/engine/apps/grafana_plugin/tests/test_gcom_api_client.py b/engine/apps/grafana_plugin/tests/test_gcom_api_client.py
index 0df6ae31..395d9b88 100644
--- a/engine/apps/grafana_plugin/tests/test_gcom_api_client.py
+++ b/engine/apps/grafana_plugin/tests/test_gcom_api_client.py
@@ -36,16 +36,16 @@ class TestIsRbacEnabledForStack:
             ({}, False),
             ({"config": {}}, False),
             ({"config": {"feature_toggles": {}}}, False),
-            ({"config": {"feature_toggles": {"enable": "foo bar baz"}}}, False),
+            ({"config": {"feature_toggles": {"enable": "foo,bar,baz"}}}, False),
             ({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "false"}}}, False),
-            # must be space separated
-            ({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE}baz"}}}, False),
+            # must be comma separated
+            ({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE}baz"}}}, False),
             # these cases will probably never happen, but lets account for them anyways
             (
                 {
                     "config": {
                         "feature_toggles": {
-                            "enable": f"foo bar baz {TEST_FEATURE_TOGGLE}",
+                            "enable": f"foo,bar,baz,{TEST_FEATURE_TOGGLE}",
                             TEST_FEATURE_TOGGLE: "false",
                         }
                     }
@@ -53,8 +53,10 @@ class TestIsRbacEnabledForStack:
                 True,
             ),
             ({"config": {"feature_toggles": {"enable": f"foo bar baz", TEST_FEATURE_TOGGLE: "true"}}}, True),
-            ({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE} baz"}}}, True),
             ({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "true"}}}, True),
+            # features enabled via feature_toggles.enable should be comma separated, not space separated
+            ({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE},baz"}}}, True),
+            ({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE} baz"}}}, False),
         ],
     )
     def test_feature_toggle_is_enabled(self, instance_info, expected) -> None: