-
Notifications
You must be signed in to change notification settings - Fork 150
fix: allow thumbnail upload if no thumbnail #2388
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
base: master
Are you sure you want to change the base?
fix: allow thumbnail upload if no thumbnail #2388
Conversation
Thanks for the pull request, @asadali145! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2388 +/- ##
=======================================
Coverage 94.63% 94.63%
=======================================
Files 1180 1180
Lines 26151 26154 +3
Branches 5836 5822 -14
=======================================
+ Hits 24747 24751 +4
- Misses 1333 1340 +7
+ Partials 71 63 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@bradenmacdonald Thanks for the review. I have updated the PR to keep the default SVG for the video icon and fix the upload. Can you please rereview this one? ![]() ![]() |
@asadali145 That looks better, thanks! But now there are a bunch of issues on systems where users are not allowed to upload thumbnails ( Screen.Recording.2025-08-22.at.3.21.52.PM.mov |
2489f7f
to
d6b9ae1
Compare
@bradenmacdonald, thanks for identifying the issue. I have updated my implementation to respect the allowThumbnailUpload flag. Could you please re-review this? |
@bradenmacdonald A reminder that this is ready for another look. |
@asadali145 Sorry for the delay. I'll try to look again soon. |
@asadali145 OK, when I have
![]() When I set Screen.Recording.2025-09-03.at.6.51.29.PM.mov |
df70429
to
086c0ff
Compare
@bradenmacdonald, I am not sure about this behavior that you see. I have tested it again and recorded the attached video. Can you please make sure that you are on my branch and the flag is set properly? Screen.Recording.2025-09-04.at.4.19.10.PM.mov |
@bradenmacdonald Would you be able to look at this today? We have a planned teak release for one of our deployments on Tuesday and would love to bring this fix in teak. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asadali145 OK, I figured out the difference.
If the video has imported
status, it works fine as shown in your screenshot. But if the video has ready
status, it doesn't work at all.
Video status is imported
:
✅ looks good.

Video status is ready
:
❌ Button to upload thumbnail does not appear, but the hover effect is still there.

To reproduce:
- Go to http://studio.local.openedx.io:8001/admin/edxval/video/ (or first http://studio.local.openedx.io:8001/admin/edxval/coursevideo/ if necessary), find the video, and change its status from
imported
toready
. - Go back to the page and notice that it's not working.
> | ||
<Icon src={VideoFile} style={{ height: '48px', width: '48px' }} /> | ||
</div> | ||
)} | ||
</div> | ||
<div className="add-thumbnail" data-testid={`video-thumbnail-${id}`}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This add-thumbnail
div with the button needs to be moved below, where the FileInput
is. In other words, we should not hide this button when showThunbnail
is false
.
{allowThumbnailUpload && (
<>
<div className="add-thumbnail" data-testid={`video-thumbnail-${id}`}>
<Button
variant="primary"
size="sm"
onClick={fileInputControl.click}
tabIndex={0}
>
{addThumbnailMessage}
</Button>
</div>
<FileInput
key="video-thumbnail-upload"
fileInput={fileInputControl}
supportedFileFormats={supportedFiles}
allowMultiple={false}
/>
</>
)}
Description
Fixes the thumbnail to allow uploads if there is no existing thumbnail
Supporting information
Testing instructions
Screenshots
Before:
https://github.com/user-attachments/assets/70cdea3e-1aa1-4035-8520-ee1e98a943cd
After:
https://github.com/user-attachments/assets/38ae3e74-b480-492b-b726-ec4aab782401