feat: add video page #640
feat: add video page #640
Conversation
KristinAoki
requested review from
kenclary,
jesperhodge,
connorhaugh and
rayzhou-bit
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #640 +/- ##
==========================================
+ Coverage 86.67% 88.02% +1.35%
==========================================
Files 389 417 +28
Lines 5799 6598 +799
Branches 1274 1428 +154
==========================================
+ Hits 5026 5808 +782
- Misses 748 765 +17
Partials 25 25
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I'm at best half-done with the review, but I want to give some early feedback.
What I see works very very well. It's a huge amount of work and has barely any problems - I found a few very minor display bugs but added comments for them. The code is also very readable and structured and I haven't discovered any problems there so far.
Your refactoring of the Files components is really great. I am still looking at that but I don't see any issue with that.
The folder structure seems a bit nested and hard to follow for me now - I left a comment about this. It's also not a big thing.
The tests you added that I have looked at so far are very good and cover a lot. I generally like that you test many components together in an integration test fashion. I think there is still a lot that is not covered, however; I so far (I mainly looked at TranscriptSettings tests) only see testing for the "happy path". Besides checking what happens when you get 500 or error responses, I think there's need for testing a lot of edge cases.
UPDATE:
I wasn't correct in saying you don't test for edge cases/unit tests. The tests for api.js cover edge cases very well, my comment was directed at the transcript settings folder.
src/files-and-videos/videos/transcript-settings/TranscriptSettings.test.jsx
Outdated
Show resolved
Hide resolved
src/files-and-videos/videos/transcript-settings/OrderTranscriptForm.jsx
Outdated
Show resolved
Hide resolved
src/files-and-videos/videos/transcript-settings/OrderTranscriptForm.jsx
Outdated
Show resolved
Hide resolved
src/files-and-videos/videos/transcript-settings/OrderTranscriptForm.jsx
Outdated
Show resolved
Hide resolved
src/files-and-videos/videos/transcript-settings/Cielo24Form.jsx
Outdated
Show resolved
Hide resolved
| jest.mock('file-saver'); | ||
|
|
||
| describe('api.js', () => { | ||
| describe('getDownload', () => { |
There was a problem hiding this comment.
The tests are great, but I only see tests for the getDownload function. It would be good to test each function in here directly or at least indirectly when they're called in another function. Covering them by testing the react components that call them I think is not be enough to cover edge cases - you see from the tests for this function alone how many possible edge cases there are.
I don't think this needs to account for every possible edge case at all, but just the most important ones.
There was a problem hiding this comment.
I think that only the getDownload has a lot of edge cases because of the nested errors. I think that I sufficiently cover the cases in the integration tests for the other api calls.
src/files-and-videos/videos/info-sidebar/TranscriptTab.test.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is great work. As far as the tests go, you have added really good tests for edge cases and failing api requests everywhere, and according to Codecov you also have 95% test coverage and raised the coverage of the project quite a bit. This is great. Codecov does show some uncovered lines but I don't think there is a need to go for 100% in this project.
With the folder structure, the changes you made are helpful, but the organizing principle is still intransparent for me. It's not immediately obvious which components are the files and video pages, where the components belong to that are in the files-and-videos folder not in subfolders, and why components such as AssetThumbnail that are not pages themselves live in the files-page folder.
I wanted to show you other ways of organizing this, so I played around a bit and created a PR (with this as target) that reorganizes stuff in a way that looks good to me. You can have a look at it to see whether you agree it is an improvement. https://github.com/openedx/frontend-app-course-authoring/pull/660/files
Otherwise I just had the request to not update paragon right now unless absolutely necessary.
package.json
Outdated
| "@edx/frontend-platform": "5.6.1", | ||
| "@edx/paragon": "^20.45.4", | ||
| "@edx/paragon": "^21.5.6", |
There was a problem hiding this comment.
I think this is not strictly necessary, right?
I would like to defer this to a separate PR until we have figured out the library-authoring gocd pipeline.
There was a problem hiding this comment.
Note that the problem is that there is a dependency conflict with the frontend-component-footer version pinned via npm alias in https://github.com/edx/edx-internal/blob/master/frontends/frontend-app-course-authoring/stage_config.yml (as well as the prod version of this and probably edge-internal repo as well). The gocd pipeline for library-authoring ran into a new error however when I updated that, indicating that the problem is not that simple to fix.
KristinAoki
force-pushed
the
KristinAoki/video-page
branch
from
68cd2ba to
07eaac6
Compare
JIRA Ticket: TNL-10913
This PR refactors the files and uploads table and makes it more page agnostic. This PR also adds the video page.
Testing:
make sure you test these flags' behaviors
This PR is dependent on
edx-platform#33528