Site Settings: Show error notice when icon fails to upload #11043
Conversation
aduth
added
[Feature] Site Settings
| if ( media ) { | ||
| this.saveSiteIconSetting( siteId, media ); | ||
| } else { | ||
| this.props.errorNotice( translate( 'An error occurred while uploading the file' ) ); |
There was a problem hiding this comment.
Hi! I've found a possible matching string that has already been translated 23 times:
translate( 'An error occurred while uploading the file.' ) ES Score: 9.38
Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).
| @@ -105,7 +106,13 @@ class SiteIconSetting extends Component { | |||
| } | |||
|
|
|||
| MediaStore.off( 'change', checkUploadComplete ); | |||
| this.saveSiteIconSetting( siteId, media ); | |||
There was a problem hiding this comment.
Should we avoid calling this.props.receiveMedia( siteId, media ) above if we have no media?
There was a problem hiding this comment.
Should we avoid calling
this.props.receiveMedia( siteId, media )above if we have no media?
Good question. I recall I'd recently moved that line to before the isItemBeingUploaded probably to allow optimistic display of the image while it's uploading. Let me check what the impact here is and what alternatives we have...
|
Thanks for the review @youknowriad . In the last few commits, I revised the change handler to check whether the media is falsey and, if so, avoid receiving it and instead ensure that it's removed from Redux state (assuming that it's a failed upload). Aside: I found easiest way to test is to toggle "Offline" in Chrome's network tab right before clicking "Done" of cropping screen in the media modal. |
|
I don't know if you consider this blocking or not, but when I have a failed upload, the site icon is empty after that (I see the default icon), Should we keep the old site icon or something? |
|
@youknowriad Another good point. It's starting to get a little messy, but daca87f should account for this. There's an awkward dance we need to do to try to get the previous icon's media data into Redux. I think I'd like to improve this by having the |
There was a problem hiding this comment.
@aduth The QueryMedia by id is also the exact same thing I need in stats ;) I'll draft a PR with this.
btw this is working well 🚢
And unrelated to this PR, sometimes I notice that the siteicon displayed is reverted to the previous site icon even on successful updates. But I can't not reproduce consistently and it's not specific to this PR only
youknowriad
added
[Status] Ready to Merge
and removed
[Status] Needs Review
The most common cause I've seen is when the icon is updated while a I'm not sure there's a whole lot we can do about this, aside from building some complex mechanism where we decide which attributes have changed since the sites request started and preserve them. This is complicated more by the fact that the polling still lives outside Redux. Since usually Automatticians are most affected by long |
aduth
force-pushed
the
update/site-icon-failed-upload-notice
branch
from
daca87f to
a13c177
Compare
This pull request seeks to improve the handling of a failed media upload in the site icon flow. Currently if a media fails to upload, we still try to receive it into Redux state and use it as the site icon, resulting in the icon resetting to the initial value with no indication aside from a console error. The changes here make it such that if the icon upload fails, a global error notice is shown to indicate the failure, and no attempt is made to use the media as the icon.
Testing instructions:
This was discovered in testing site icon usage in the desktop application. It may also be possible to test by toggling "Offline" in Chrome Network tab at point of clicking "Done" in Site Icon flow. If desktop app is available for testing, simply change into
calypsodirectory and checkout this branch before starting desktop app withmake run.