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

Add onUpdate event listener for installing starter blueprints on Playground urls #4249

Merged
merged 21 commits into from
Sep 8, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 45 additions & 20 deletions src/background/starterBlueprints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { installStarterBlueprints } from "@/background/starterBlueprints";
import { firstTimeInstallStarterBlueprints } from "@/background/starterBlueprints";
import { loadOptions, saveOptions } from "@/store/extensionsStorage";
import MockAdapter from "axios-mock-adapter";
import axios from "axios";
import { isLinked } from "@/auth/token";
import { extensionFactory, recipeFactory } from "@/testUtils/factories";
import { PersistedExtension } from "@/core";
import { PersistedExtension, RecipeMetadata } from "@/core";

const axiosMock = new MockAdapter(axios);

Expand Down Expand Up @@ -55,10 +55,15 @@ describe("installStarterBlueprints", () => {
test("user has starter blueprints available to install", async () => {
isLinkedMock.mockResolvedValue(true);

axiosMock.onGet().reply(200, [recipeFactory()]);
axiosMock.onPost().reply(204);
axiosMock
.onGet("/api/onboarding/starter-blueprints/install/")
.reply(200, { install_starter_blueprints: true });
axiosMock
.onGet("/api/onboarding/starter-blueprints/")
.reply(200, [recipeFactory()]);
axiosMock.onPost("/api/onboarding/starter-blueprints/install/").reply(204);

await installStarterBlueprints();
await firstTimeInstallStarterBlueprints();
const { extensions } = await loadOptions();

expect(extensions.length).toBe(1);
Expand All @@ -68,10 +73,15 @@ describe("installStarterBlueprints", () => {
test("user does not have starter blueprints available to install", async () => {
isLinkedMock.mockResolvedValue(true);

axiosMock.onGet().reply(200, []);
axiosMock.onPost().reply(204);
axiosMock
.onGet("/api/onboarding/starter-blueprints/install/")
.reply(200, { install_starter_blueprints: false });
axiosMock
.onGet("/api/onboarding/starter-blueprints/")
.reply(200, [recipeFactory()]);
axiosMock.onPost("/api/onboarding/starter-blueprints/install/").reply(204);

await installStarterBlueprints();
await firstTimeInstallStarterBlueprints();
const { extensions } = await loadOptions();

expect(extensions.length).toBe(0);
Expand All @@ -81,10 +91,13 @@ describe("installStarterBlueprints", () => {
test("starter blueprints request fails", async () => {
isLinkedMock.mockResolvedValue(true);

axiosMock.onGet().reply(500);
axiosMock.onPost().reply(204);
axiosMock
.onGet("/api/onboarding/starter-blueprints/install/")
.reply(200, { install_starter_blueprints: true });
axiosMock.onGet("/api/onboarding/starter-blueprints/").reply(500);
axiosMock.onPost("/api/onboarding/starter-blueprints/install/").reply(204);

await installStarterBlueprints();
await firstTimeInstallStarterBlueprints();
const { extensions } = await loadOptions();

expect(extensions.length).toBe(0);
Expand All @@ -94,10 +107,15 @@ describe("installStarterBlueprints", () => {
test("starter blueprints installation request fails", async () => {
isLinkedMock.mockResolvedValue(true);

axiosMock.onGet().reply(200, []);
axiosMock.onPost().reply(500);
axiosMock
.onGet("/api/onboarding/starter-blueprints/install/")
.reply(200, { install_starter_blueprints: true });
axiosMock
.onGet("/api/onboarding/starter-blueprints/")
.reply(200, [recipeFactory()]);
axiosMock.onPost("/api/onboarding/starter-blueprints/install/").reply(500);

await installStarterBlueprints();
await firstTimeInstallStarterBlueprints();
const { extensions } = await loadOptions();

expect(extensions.length).toBe(0);
Expand All @@ -107,22 +125,29 @@ describe("installStarterBlueprints", () => {
test("starter blueprint already installed", async () => {
isLinkedMock.mockResolvedValue(true);

const extension = extensionFactory() as PersistedExtension;
const recipe = recipeFactory();

const extension = extensionFactory({
_recipe: { id: recipe.metadata.id } as RecipeMetadata,
}) as PersistedExtension;
await saveOptions({
extensions: [extension],
});

axiosMock.onGet().reply(200, [
axiosMock
.onGet("/api/onboarding/starter-blueprints/install/")
.reply(200, { install_starter_blueprints: true });

axiosMock.onGet("/api/onboarding/starter-blueprints/").reply(200, [
{
updated_at: "",
extensionPoints: [extension],
sharing: {},
...recipe,
},
]);

axiosMock.onPost().reply(204);
axiosMock.onPost("/api/onboarding/starter-blueprints/install/").reply(204);

await installStarterBlueprints();
await firstTimeInstallStarterBlueprints();
const { extensions } = await loadOptions();

expect(extensions.length).toBe(1);
Expand Down
139 changes: 106 additions & 33 deletions src/background/starterBlueprints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,74 +23,147 @@ import { forEachTab } from "@/background/util";
import { queueReactivateTab } from "@/contentScript/messenger/api";
import { ExtensionOptionsState } from "@/store/extensionsTypes";
import reportError from "@/telemetry/reportError";
import { debounce } from "lodash";

const { reducer, actions } = extensionsSlice;

function installStarterBlueprint(
const PLAYGROUND_URL = "https://www.pixiebrix.com/playground";
let isInstallingBlueprints = false;
const BLUEPRINT_INSTALLATION_DEBOUNCE_MS = 10_000;
const BLUEPRINT_INSTALLATION_MAX_MS = 60_000;

function installBlueprint(
state: ExtensionOptionsState,
starterBlueprint: RecipeDefinition
blueprint: RecipeDefinition
): ExtensionOptionsState {
return reducer(
state,
actions.installRecipe({
recipe: starterBlueprint,
extensionPoints: starterBlueprint.extensionPoints,
recipe: blueprint,
extensionPoints: blueprint.extensionPoints,
})
);
}

export async function installStarterBlueprints(): Promise<void> {
async function installBlueprints(
Copy link
Contributor

@fregante fregante Sep 8, 2022

Choose a reason for hiding this comment

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

To ensure that it's not called undebounced (as it currently is on line 150), we can prepend _ to the name.

I generally inline the function in the debounce definition (debounce(() => {}, opts)) but for long functions it's less readable.

Suggested change
async function installBlueprints(
async function _installBlueprints(

blueprints: RecipeDefinition[]
): Promise<boolean> {
let installed = false;
if (blueprints.length === 0) {
return installed;
}

let extensionsState = await loadOptions();
for (const blueprint of blueprints) {
const blueprintAlreadyInstalled = extensionsState.extensions.some(
(extension) => extension._recipe.id === blueprint.metadata.id
);

if (!blueprintAlreadyInstalled) {
extensionsState = installBlueprint(extensionsState, blueprint);
installed = true;
}
}

await saveOptions(extensionsState);
await forEachTab(queueReactivateTab);
return installed;
}

async function getShouldFirstTimeInstall(): Promise<boolean> {
const client = await maybeGetLinkedApiClient();
if (client == null) {
console.debug(
"Skipping starter blueprint installation because the extension is not linked to the PixieBrix service"
);
return;
return false;
}

try {
const {
data: { install_starter_blueprints: shouldInstall },
} = await client.get("/api/onboarding/starter-blueprints/install/");

if (shouldInstall) {
// If the starter blueprint request fails for some reason, or the user's primary organization
// gets removed, we'd still like to mark starter blueprints as installed for this user
// so that they don't see onboarding views/randomly have starter blueprints installed
// the next time they open the extension
await client.post("/api/onboarding/starter-blueprints/install/");
}

return shouldInstall;
} catch (error) {
reportError(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is going to be returned from this function when it catches an error? I think void will be coerced to false, but that's not really clear from looking at the code. Can we please return false here explicitly to make it more clear?

Copy link
Contributor

@fregante fregante Sep 7, 2022

Choose a reason for hiding this comment

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

👍 TypeScript in loose mode misses a lot of such warnings. It should have complained that the signature is boolean but there's no return here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this Ben! This was indeed a mistake.

return false;
}
}

async function getStarterBlueprints(): Promise<RecipeDefinition[]> {
const client = await maybeGetLinkedApiClient();
if (client == null) {
console.debug(
"Skipping starter blueprint installation because the extension is not linked to the PixieBrix service"
);
return [];
Copy link
Contributor

Choose a reason for hiding this comment

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

💙

}

try {
const { data: starterBlueprints } = await client.get<RecipeDefinition[]>(
"/api/onboarding/starter-blueprints/"
);
return starterBlueprints;
} catch (error) {
reportError(error);
return [];
}
}

// If the starter blueprint request fails for some reason, or the user's primary organization
// gets removed, we'd still like to mark starter blueprints as installed for this user
// so that they don't see onboarding views/randomly have starter blueprints installed
// the next time they open the extension
await client.post("/api/onboarding/starter-blueprints/install/");

if (starterBlueprints.length === 0) {
return;
}
const installStarterBlueprints = async (): Promise<boolean> => {
if (isInstallingBlueprints) {
return false;
}

let extensionsState = await loadOptions();
isInstallingBlueprints = true;
const starterBlueprints = await getStarterBlueprints();
const installed = await installBlueprints(starterBlueprints);
isInstallingBlueprints = false;
return installed;
};

for (const starterBlueprint of starterBlueprints) {
const blueprintAlreadyInstalled = extensionsState.extensions.some(
(extension) => extension._recipe.id === starterBlueprint.metadata.id
);
const debouncedInstallStarterBlueprints = debounce(
installStarterBlueprints,
BLUEPRINT_INSTALLATION_DEBOUNCE_MS,
{
leading: true,
trailing: false,
maxWait: BLUEPRINT_INSTALLATION_MAX_MS,
}
);

if (!blueprintAlreadyInstalled) {
extensionsState = installStarterBlueprint(
extensionsState,
starterBlueprint
);
}
}
export async function firstTimeInstallStarterBlueprints(): Promise<void> {
const shouldInstall = await getShouldFirstTimeInstall();
if (!shouldInstall) {
return;
}

await saveOptions(extensionsState);
const installed = await installStarterBlueprints();
Copy link
Contributor

@fregante fregante Sep 8, 2022

Choose a reason for hiding this comment

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

While unlikely, it could be called concurrently with browser.tabs.onUpdated. It doesn't hurt to use the debounced version everywhere to avoid these leaks


await forEachTab(queueReactivateTab);
if (installed) {
void browser.tabs.create({
url: "https://www.pixiebrix.com/playground",
url: PLAYGROUND_URL,
});
} catch (error) {
reportError(error);
}
}

function initStarterBlueprints(): void {
void installStarterBlueprints();
browser.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
if (tab?.url?.startsWith(PLAYGROUND_URL)) {
void debouncedInstallStarterBlueprints();
}
});

void firstTimeInstallStarterBlueprints();
}

export default initStarterBlueprints;