fix(manager, slice-machine-ui, playwright): environment edge cases

[angeloashmore]

Context

Fixes DT-1787, DT-1861, DT-1877

The Solution

Use manager.project.fetchActiveEnvironment() to fetch the active environment from the manager. Previously, Slice Machine UI would filter the list of environments on the client.

If the active environment is invalid (not a real environment or with insufficient access), throw an error. This error is caught in SM UI, triggering the UI to reset the environment back to production. In the future, a toast can appear informing the user of the change.

Side fixes

• This PR changes how Playwright tests are written for the environment selector.
• This PR re-introduces client-side error checking with the is____Error() helpers.

Impact / Dependencies

• Users with environments with user permissions will now show the correct list of accessible environments.
• The edge case of an invalid environment is now handled.

Checklist before requesting a review

• I hereby declare my code ready for review.
• If it is a critical feature, I have added tests.
• The CI is successful.
• If there could backward compatibility issues, it has been discussed and planned.

[linear]

DT-1877 AAUser, setting an invalid environment resets my environment to production

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
slice-machine	✅ Ready (Inspect)	Visit Preview	Dec 26, 2023 10:58pm

[linear]

DT-1861 AAUser, I only see environments I can access

[linear]

DT-1787 Audit and update/fix isUnauthorizedError and isUnauthenticatedError

[xrutayisire]

Hey, super nice improvements! Totally in sync with what we missed for the first release with envs! Super work! I let you general feedbacks, nothing is mandatory, I let you tell me what you think 🙂

[xrutayisire]

Can this create an infinite loop here?

[angeloashmore]

It could if updateEnvironment({ environment: undefined }) continued to set an invalid environment. That should only happen if the adapter does not adhere to the convention that undefined is the production environment.

Do you have a suggestion on handling it? We could attempt changing to the production environment once and fail hard if it does not change.

I'm not sure how we handle the failure case, however. It theoretically shouldn't happen, so a full-app error message may be okay.

[xrutayisire]

For me, we should have only one fallback to "Production", yes I guess.

[xrutayisire]

My other proposal was to handle an array of procedures where you can define twice the same procedure path. Like that if you define it 2 times, the first one will be the first call, the second one will be the second call + all next. WDYT?
See: https://github.com/prismicio/slice-machine/blob/53b09cc308e1fe1d8e816b1025833cabaf1a2a78/playwright/utils/mockManagerProcedures.ts

[angeloashmore]

Something like MSW's fall-through behavior would be nice. It's similar to what you are describing, but a little more flexible and natural-feeling.

Basically, if a route interceptor returns a Response instance, the response is used. If a Response isn't returned, the next handler for that route is used. It continues until a Response is found, or fails to handle the request if none are there. The handlers can be set to only be used once.

We can't do that exact system in our case, but we can do something similar:

• Maintain a list of handlers.
• Every time mockManagerProcedures() is called, the handlers are added to the start of the list.
• The first matching handler is used.
• Handlers can be configured to be removed after being used once.

This allows us to set up procedure handlers at the start of the test and add/modify new ones throughout the test. It also allows us to mock behavior that happens without user interaction (like the above didMockFetchActiveEnvironmentOnce logic).

This improvement would also remove the need for the buildMockEnvironmentProcedures() helper I wrote, which was needed to cut down on the repetitiveness and verbosity.

[angeloashmore]

It means we can write tests like this:

  test.run()(
    "I can see the window top border depending on the environment",
    async ({ sliceMachinePage }) => {
      await mockManagerProcedures({
        page: sliceMachinePage.page,
        procedures: [
          {
            path: "prismicRepository.fetchEnvironments",
            data: () => ({ environments }),
            execute: false,
          },
        ],
      });

      await sliceMachinePage.gotoDefaultPage();

      await mockManagerProcedures({
        page: sliceMachinePage.page,
        procedures: [
          {
            path: "prismicRepository.fetchActiveEnvironment",
            data: () => ({ activeEnvironment: environments[0] }),
            execute: false,
          },
        ],
      });
      await sliceMachinePage.menu.environmentSelector.selectEnvironment(
        environments[0].name,
      );
      await expect(sliceMachinePage.topBorder).toHaveCSS(
        "background-color",
        "rgb(109, 84, 207)",
      );

      await mockManagerProcedures({
        page: sliceMachinePage.page,
        procedures: [
          {
            path: "prismicRepository.fetchActiveEnvironment",
            data: () => ({ activeEnvironment: environments[1] }),
            execute: false,
          },
        ],
      });
      await sliceMachinePage.menu.environmentSelector.selectEnvironment(
        environments[1].name,
      );
      await expect(sliceMachinePage.topBorder).toHaveCSS(
        "background-color",
        "rgb(56, 91, 204)",
      );

      await mockManagerProcedures({
        page: sliceMachinePage.page,
        procedures: [
          {
            path: "prismicRepository.fetchActiveEnvironment",
            data: () => ({ activeEnvironment: environments[2] }),
            execute: false,
          },
        ],
      });
      await sliceMachinePage.menu.environmentSelector.selectEnvironment(
        environments[2].name,
      );
      await expect(sliceMachinePage.topBorder).toHaveCSS(
        "background-color",
        "rgb(255, 159, 26)",
      );
    },
  );

[angeloashmore]

@xrutayisire and I discussed it off GitHub and came to the conclusion that an API like this would be ideal:

procedures.mock("project.fetchActiveEnvironment", ({ data, args }) => {
  // ...
}, {
  execute: false,
  times: 1
})

procedures would be a fixture with a stateful list of procedures for the test. As mock() is called, handlers are added to the list.

It will be implemented in a separate PR and ported to these tests.

[xrutayisire]

Generally, did you test it with an alpha? Because last time it was not possible to see the problem in Dev mode but only with a full SM build :/

[angeloashmore]

I did not, but I can try it easily now since we have the new release script. 🙂

[xrutayisire]

Personally, I think wouldn't have done that, it creates a bit more complexity when reading tests. I had to check above when you slice or take only one, which one. What does it mean, etc.

I feel the repetition super light here, no? If you just have a simple array most of the time but yes you repeat the path, I think it's easier to maintain and to read.

To maintain shared small function like this, we can break all tests while just wanting to update one. I let you see what you think if it's flatten and judge it. I just want us to try to have less overhead when reading tests, even if it means a bit of repetition. And since any failure / problem will actually make the test fail it's easy to maintain.

[angeloashmore]

I agree; I'm not a big fan of builder functions like this, especially because eventually that function will grow to support more configuration rather than inlining the contents where it is used.

The repetition comes from the fetchEnvironments mock. It needs to be repeated every time we call mockManagerProcedures; only the most recent mockManagerProcedures call's procedures seemed to be registered.

Because the environment switching logic is reused in several tests, the file became long and repetitive.

If we can get it to work, this is a nicer solution in my opinion: #1253 (comment)