MERC-8592 Fix stencil CLI Push resulting in intermittent failures

[powerwlsl]

What?

UPDATED
There are 2 bugs behind this issue.

1. When posting a new theme, it sometimes returns 409 because the theme that was supposed to be deleted is still not done at that point. To resolve this issue, I added checkIfDeletionIsComplete to check if the theme deletion is done before uploading a new theme. This will prevent the 409 error that we saw on kibana.

As you can see from the screenshot above, it will check if the theme that is supposed to be deleted still exists before pushing a new theme

2. The 409 error above is not caught in the right place, so it fails at the wrong place.
   The fix above (adding checkIfDeletionIsComplete) will fix this issue automatically, since 409 won't happen when uploading a theme. Still, I think it's a good idea to fix this issue since it's not catching the error in the right place.

I found that when the 409 error occurs, there's no job id passed to the getJob method. And this causes an error when fetching a job because it's trying to fetch a job with anundefined job id.

These are the process that happens when you push a theme.

            async.constant(options),
            utils.readStencilConfigFile,
            utils.getStoreHash,
            utils.getThemes,
            utils.generateBundle,
            utils.uploadBundle,      <===== First upload trial
            utils.notifyUserOfThemeLimitReachedIfNecessary,  <===== Prompt user to delete a theme if it reaches the limit
            utils.promptUserToDeleteThemesIfNecessary,
            utils.deleteThemesIfNecessary,
            utils.uploadBundleAgainIfNecessary, <===== Second upload trial
            utils.notifyUserOfThemeUploadCompletion,
            utils.pollForJobCompletion((data) => ({ themeId: data.theme_id })), <===== This is where the error happens
            utils.promptUserWhetherToApplyTheme,
            utils.getChannels,
            utils.promptUserForChannels,
            utils.getVariations,
            utils.promptUserForVariation,
            utils.requestToApplyVariationWithRetrys(),
            utils.notifyUserOfCompletion,

As you can see, after the first upload theme trial, it returns 409 if the number of themes reaches the limit. When that happens, it goes to notifyUserOfThemeLimitReachedIfNecessary and asks the user to delete one of their themes.

After the user deletes a theme, it goes to the second upload trial and that's when this bug happens. If when there's 409 error, instead of raising an error, it returns { themeLimitReached: true } which leaves the job_id as undefined. And since there's no job id, pollForJobCompletion will fail while fetching a job status.

Tickets / Documentation

Add links to any relevant tickets and documentation.

https://bigcommercecloud.atlassian.net/browse/MERC-8592

cc @bigcommerce/storefront-team

[jairo-bc]

@powerwlsl Does this mean, that when a developer (merchant) reached to a theme upload limit ( btw, which is that currently ?), on retry he will get to https://github.com/bigcommerce/stencil-cli/pull/935/files#diff-e63742395d19241c3e98c9af0e621e344d108b3a019c3eb72b70162f1e6a101fR318.

What will be the error message in that case? Do you have screenshots with new behaviour?

What do you think about removing error message handling from postTheme and leave the server response flow to the uploadBundle?

[powerwlsl]

Ji @jairo-bc, the PR has been updated. We found that 409 is caused because the theme that is supposed to be deleted has not been deleted before posting a new theme. So i added a fix for that within this PR. (check out the PR description 1. above)

To answer your question, when a merchant reaches to a theme upload limit, this is the behavior that they will see.

1. if they push a theme with the -d option, it will automatically delete the oldest theme and continue pushing a new theme after the theme is completely deleted.

2.if they push a theme without the -d option, it will prompt user to delete one of their themes and then continue pushing a new theme after the theme is completely deleted.

[powerwlsl]

@jairo-bc
When do you mean by this ?

What do you think about removing error message handling from postTheme and leave the server response flow to the uploadBundle?

Can you elaborate on this a little bit more?

[jairo-bc]

@powerwlsl I mean this 409 error message handling https://github.com/bigcommerce/stencil-cli/blob/master/lib/theme-api-client.js#L310. Do we need it? It would be nice to have theme registry provide human readable error messages

[powerwlsl]

@jairo-bc Hmm I think how we do error handling is ok as is since we cover other errors (413, 201) the same way in the same function. But if you strongly feel to change it, we can consider doing it in a different ticket.

[bc-jcha]

Talked offline. After we check the deletion status 5 times, the code is going to stop checking and fail with 409. We need to make sure to surface the proper error (deletion still in process) in this case.

[powerwlsl]

thanks for the code review! @bc-jcha

actually, it turns out the error raised is covered by a function called printCliResultError, so it doesn't go all the way and fail on 409.

So i think it's good to leave it as is. Or maybe we can change the error message more user friendly with better explanation.