RiotGames / key-conjurer

Temporary Credential Service
https://technology.riotgames.com/news/key-conjurer-our-policy-least-privilege
Apache License 2.0
167 stars 33 forks source link

Adding accounts select placeholder [Tiny improvement at the UI] #87

Closed leonardodimarchi closed 1 year ago

leonardodimarchi commented 1 year ago

Added accounts select placeholder when the account is not selected.

Tested by running the unit tests and updating the accounts at the store to manually check if the select is working as expected

Before: image

After: image

punmechanic commented 1 year ago

Hey, I'm gonna accept this after I add a javascript formatter to the repo.

punmechanic commented 1 year ago

Hey, so this was merged in faf0630 after I added some CI. It does look like this broke two tests which I have resolved in 8829895. Next time, you'll get alerted in the PR if you merge something that breaks tests.

Specifically the following assertions were broken:

test("should change account to the first account in list if one has been selected when the account list changes, if the new account list does not include the currently selected account", async () => {
  const user = userEvent.setup();
  const { getByLabelText } = render(<KeyRequestForm />);
  update("idpInfo", {
    apps: [
      { name: "Account #1", id: 0 },
      { name: "Account #2", id: 1 },
      { name: "Account #3", id: 2 },
    ],
  });

  const select = getByLabelText("Account");
  await user.selectOptions(select, "Account #2");
  expect(select.value).toEqual("1");

  update("idpInfo", {
    apps: [
      { name: "Account #1", id: 0 },
      { name: "Account #3", id: 2 },
    ],
  });

  expect(select.value).toEqual("0");
});

The new behavior causes select.value to equal "Accounts" on the second assertion. Given the context of the test, this is more correct than what it was doing previously, which is selecting the first option in the list if you had already selected an option, and that option was removed.

Similarly, this test broke:

test("should change account to first account in list when the account list is changed", () => {
  const { getByLabelText } = render(<KeyRequestForm />);
  update("idpInfo", { apps: [] });

  const select = getByLabelText("Account");
  expect(select.value).toEqual("");

  update("idpInfo", {
    apps: [
      { name: "Account #3", id: 2 },
      { name: "Account #2", id: 1 },
      { name: "Account #1", id: 0 },
    ],
  });

  expect(select.value).toEqual("2");
});

The first assertion here is "No Accounts" now, which, again, makes sense.

leonardodimarchi commented 1 year ago

Hey Dan, thank you for the explanation. i was sure i ran these tests after making the changes, I'll pay more attention in the next PRs