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.

Sign up for GitHub

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

upload files icon #2868

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

upload files icon #2868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions src/cmd/sign.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export default function sign(
channel,
amoMetadata,
uploadSourceCode,
amoIcon,
webextVersion,
},
{
Expand Down Expand Up @@ -154,6 +155,7 @@ export default function sign(
approvalCheckTimeout:
approvalTimeout !== undefined ? approvalTimeout : timeout,
submissionSource: uploadSourceCode,
amoIcon,
});
} else {
const {
Expand Down
7 changes: 7 additions & 0 deletions src/program.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,13 @@ Example: $0 --help run.
'details. Only used with `use-submission-api`',
type: 'string',
},
'amo-icon': {
describe:
'Path to an image that should be displayed as the addon icon on ' +
'addons.mozilla.org. Must be square; 128px x 128px is recommended. ' +
'Only used with `use-submission-api`',
type: 'string',
},
},
)
.command('run', 'Run the extension', commands.run, {
Expand Down
61 changes: 37 additions & 24 deletions src/util/submit-addon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,11 @@ export default class Client {
}

async doFormDataPatch(data, addonId, versionId) {
const patchUrl = new URL(
`addon/${addonId}/versions/${versionId}/`,
this.apiUrl,
);
let patchUrl = new URL(`addon/${addonId}/`, this.apiUrl);
if (versionId) {
patchUrl = new URL(`versions/${versionId}/`, patchUrl);
}

try {
const formData = new FormData();
for (const field in data) {
Expand All @@ -207,10 +208,19 @@ export default class Client {
}

async doAfterSubmit(addonId, newVersionId, editUrl, patchData) {
const promises = [];
if (patchData && patchData.version) {
log.info(`Submitting ${Object.keys(patchData.version)} to version`);
await this.doFormDataPatch(patchData.version, addonId, newVersionId);
promises.push(
this.doFormDataPatch(patchData.version, addonId, newVersionId),
);
}
if (patchData && patchData.addon) {
log.info(`Submitting ${Object.keys(patchData.addon)} to addon`);
promises.push(this.doFormDataPatch(patchData.addon, addonId));
}
await Promise.all(promises);
Copy link
Member

Choose a reason for hiding this comment

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

The version-specific (source code) data and addon-global (icon) data are independent. A failure in any of these can result in an early termination of the logic. Is that desirable?

Instead of termination as soon as any failure occurs, it may make sense to wrap individual promises in always-fulfilling promises and doing error handling there.

Alternatively, if you'd like to report errors late, use Promise.allSettled, and then check whether there are any errors (and print useful information if needed).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think, broadly, proceeding with a warning is probably the way forward - because at that point they can't fix the problem without a whole new version being uploaded (with a version number bump).

The downside is there isn't a function in web-ext to make arbitrary API calls without uploading a versions. So the "fix" would be for them to either make a manual API request (difficult, without the tooling to make the jwt for the Auth); or use devhub (a different UX to command line). And yeah, they might miss the message in the other logging if there isn't a hard error thrown.

Given this is a flaw with the source code upload too, we should probably address that as a separate issue first (blocking V8?)


if (this.approvalCheckTimeout > 0) {
const fileUrl = new URL(
await this.waitForApproval(addonId, newVersionId),
Expand Down Expand Up @@ -415,19 +425,16 @@ export async function signAddon({
savedUploadUuidPath,
metaDataJson = {},
submissionSource,
amoIcon,
userAgentString,
SubmitClient = Client,
ApiAuthClass = JwtApiAuth,
}) {
try {
const stats = await fsPromises.stat(xpiPath);

if (!stats.isFile()) {
throw new Error('not a file');
}
} catch (statError) {
throw new Error(`error with ${xpiPath}: ${statError}`);
}
await Promise.all([
checkPathIsFile(xpiPath),
submissionSource ? checkPathIsFile(submissionSource) : Promise.resolve(),
amoIcon ? checkPathIsFile(amoIcon) : Promise.resolve(),
]);

let baseUrl;
try {
Expand All @@ -451,19 +458,13 @@ export async function signAddon({
savedUploadUuidPath,
);
const patchData = {};
// if we have a source file we need to upload we patch after the create
// if we have a source or icon file we need to upload we patch after the create
if (submissionSource) {
try {
const stats2 = await fsPromises.stat(submissionSource);

if (!stats2.isFile()) {
throw new Error('not a file');
}
} catch (statError) {
throw new Error(`error with ${submissionSource}: ${statError}`);
}
patchData.version = { source: client.fileFromSync(submissionSource) };
}
if (amoIcon) {
patchData.addon = { icon: client.fileFromSync(amoIcon) };
}

// We specifically need to know if `id` has not been passed as a parameter because
// it's the indication that a new add-on should be created, rather than a new version.
Expand Down Expand Up @@ -526,3 +527,15 @@ export async function getUploadUuidFromFile(

return { uploadUuid: '', channel: '', xpiCrcHash: '' };
}

async function checkPathIsFile(filePath) {
try {
const stats2 = await fsPromises.stat(filePath);

if (!stats2.isFile()) {
throw new Error('not a file');
}
} catch (statError) {
throw new Error(`error with ${filePath}: ${statError}`);
}
}
76 changes: 75 additions & 1 deletion tests/unit/test-util/test.submit-addon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,44 @@ describe('util.submit-addon', () => {
);
});

it('includes icon data to be patched if amoIcon defined for new addon', async () => {
const amoIcon = 'path/to/icon/image';
statStub.onSecondCall().resolves({ isFile: () => true });
await signAddon({
...signAddonDefaults,
amoIcon,
});

sinon.assert.calledWith(fileFromSyncStub, amoIcon);
sinon.assert.calledWith(
postNewAddonStub,
uploadUuid,
signAddonDefaults.savedIdPath,
{},
{ addon: { icon: fakeFileFromSync } },
);
});

it('includes icon data to be patched if amoIcon defined for new version', async () => {
const amoIcon = 'path/to/icon/image';
statStub.onSecondCall().resolves({ isFile: () => true });
const id = '@thisID';
await signAddon({
...signAddonDefaults,
amoIcon,
id,
});

sinon.assert.calledWith(fileFromSyncStub, amoIcon);
sinon.assert.calledWith(
putVersionStub,
uploadUuid,
id,
{},
{ addon: { icon: fakeFileFromSync } },
);
});

it('throws error if submissionSource is not found', async () => {
const submissionSource = 'path/to/source/zip';
const signAddonPromise = signAddon({
Expand Down Expand Up @@ -1041,7 +1079,6 @@ describe('util.submit-addon', () => {
const downloadUrl = 'https://a.download/url';
const newVersionId = sampleVersionDetail.id;
const editUrl = sampleVersionDetail.editUrl;
const patchData = { version: { source: 'somesource' } };

let approvalStub;
let downloadStub;
Expand Down Expand Up @@ -1079,8 +1116,45 @@ describe('util.submit-addon', () => {

it('calls doFormDataPatch if patchData.version is defined', async () => {
client.approvalCheckTimeout = 0;
const patchData = { version: { source: 'somesource' } };
await client.doAfterSubmit(addonId, newVersionId, editUrl, patchData);

sinon.assert.calledOnce(doFormDataPatchStub);
sinon.assert.calledWith(
doFormDataPatchStub,
patchData.version,
addonId,
newVersionId,
);
});

it('calls doFormDataPatch if patchData.addon is defined', async () => {
client.approvalCheckTimeout = 0;
const patchData = { addon: { icon: 'someimage' } };
await client.doAfterSubmit(addonId, newVersionId, editUrl, patchData);

sinon.assert.calledOnce(doFormDataPatchStub);
sinon.assert.calledWith(
doFormDataPatchStub,
patchData.addon,
addonId,
);
});

it('calls doFormDataPatch twice if patchData.addon and patchData.version is defined', async () => {
client.approvalCheckTimeout = 0;
const patchData = {
version: { source: 'somesource' },
addon: { icon: 'someimage' },
};
await client.doAfterSubmit(addonId, newVersionId, editUrl, patchData);

sinon.assert.callCount(doFormDataPatchStub, 2);
sinon.assert.calledWith(
doFormDataPatchStub,
patchData.addon,
addonId,
);
sinon.assert.calledWith(
doFormDataPatchStub,
patchData.version,
Expand Down