Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Template variables: Default to first option if URL does not provide #36839

Closed
wants to merge 3 commits into from

Conversation

tskarhed
Copy link
Contributor

@tskarhed tskarhed commented Jul 16, 2021

Which issue(s) this PR fixes:

Fixes #35471

Special notes for your reviewer:

I'm currently looking at writing tests.

@tskarhed tskarhed requested review from a team, jackw and ashharrison90 and removed request for a team July 16, 2021 09:57
@tskarhed
Copy link
Contributor Author

Am I missing the reason why the selected option needs to have ""?

 when setOptionFromUrl is dispatched with a custom variable (no refresh property) › and urlValue is undefined then correct actions are dispatched

    expect(received).toEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 2

    @@ -2,12 +2,12 @@
        Object {
          "payload": Object {
            "data": Object {
              "option": Object {
                "selected": false,
    -           "text": "",
    -           "value": "",
    +           "text": "A",
    +           "value": "A",
              },
            },
            "id": "0",
            "type": "custom",
          },

      110 |     }
      111 |
    > 112 |     expect(dispatchedActions).toEqual(actions);
          |                               ^
      113 |     return instance;
      114 |   };
      115 |

      at Object.thenDispatchedActionsShouldEqual (public/test/core/redux/reduxTester.ts:112:31)
      at public/app/features/variables/state/setOptionFromUrl.test.ts:37:18
      at step (node_modules/tslib/tslib.js:143:27)
      at Object.next (node_modules/tslib/tslib.js:124:57)
      at fulfilled (node_modules/tslib/tslib.js:114:62)

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm, but does it solve the problem specified in #35471 (comment)?

seems the problem stated there is that it's defaulting to the first option instead of the option saved in the json. as far as i can tell, this will also default to the first option? it's definitely better than being blank, but idk if it solves the original problem.

@tskarhed
Copy link
Contributor Author

code lgtm, but does it solve the problem specified in #35471 (comment)?

seems the problem stated there is that it's defaulting to the first option instead of the option saved in the json. as far as i can tell, this will also default to the first option? it's definitely better than being blank, but idk if it solves the original problem.

You're right. I'm not sure how I would approach that though. Defaulting to the first option seems like a better thing than having it blank.

Maybe merge it like this for now and then come back to it?

@ashharrison90
Copy link
Contributor

oh yeah absolutely, it's definitely a step forward 🥳

@natellium natellium added this to In Review (max internal 8, max external 3) in User essentials squad (deprecated) via automation Jul 22, 2021
@ashharrison90 ashharrison90 moved this from In Review (max internal 8, max external 3) to Blocked in User essentials squad (deprecated) Jul 22, 2021
Copy link
Contributor

@hugohaggmark hugohaggmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to find a better solution to the root issue here
#35471 (comment)

@hugohaggmark hugohaggmark moved this from Blocked to In progress (max. 4) in User essentials squad (deprecated) Aug 10, 2021
@hugohaggmark hugohaggmark moved this from In progress (max. 4) to Blocked in User essentials squad (deprecated) Aug 10, 2021
@hugohaggmark
Copy link
Contributor

Superseeded by #37776

User essentials squad (deprecated) automation moved this from Blocked to Done Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Template Variable Defaults Blanked When Using Data Links Back to Same Dashboard
4 participants