feat: [FC-0044] Textbooks Page #890

vladislavkeblysh · 2024-03-12T19:08:49Z

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/raccoongang/edx-platform.git
EDX_PLATFORM_VERSION: g-glugovskiy/change_path_to_textbooks_page

TUTOR_GROVE_WAFFLE_FLAGS:
  - name: contentstore.new_studio_mfe.use_new_textbooks_page
    everyone: true

Description
This PR adds fully implemented Texbooks page.

Issue: openedx/platform-roadmap#319

Design
Figma design

Testing instructions

Run master devstack.
Start platform make dev.up.lms+cms + frontend-app-course-authoring and make checkout on this branch.
Enable the new Certificates page by adding a waffle flag contentstore.new_studio_mfe.use_new_textbooks_page in the CMS admin panel.
Go to the Textbooks page from the Content > Pages & Resourses.

Screen.Recording.2024-03-14.at.19.08.40.mov

openedx-webhooks · 2024-03-12T19:08:54Z

Thanks for the pull request, @vladislavkeblysh! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2024-03-12T19:17:34Z

Codecov Report

Attention: Patch coverage is 94.20290% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 92.20%. Comparing base (e24fb78) to head (6e5d91c).
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/textbooks/hooks.jsx	75.00%	6 Missing ⚠️
src/generic/modal-dropzone/useModalDropzone.jsx	71.42%	2 Missing ⚠️
src/textbooks/textbook-card/TextbooksCard.jsx	91.30%	2 Missing ⚠️
src/generic/modal-dropzone/ModalDropzone.jsx	50.00%	1 Missing ⚠️
src/textbooks/textbook-form/TextbookForm.jsx	96.15%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
+ Coverage   92.09%   92.20%   +0.10%     
==========================================
  Files         685      703      +18     
  Lines       12090    12334     +244     
  Branches     2609     2667      +58     
==========================================
+ Hits        11134    11372     +238     
- Misses        920      926       +6     
  Partials       36       36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mphilbrick211 · 2024-03-13T14:32:47Z

Hi @vladislavkeblysh! Would you mind adding a description of your changes to the top of the pull request?

arbrandes · 2024-03-13T14:59:50Z

You'll probably also want to make this a draft until it's ready.

vladislavkeblysh · 2024-03-13T16:43:09Z

@mphilbrick211 @arbrandes Yes, sorry, PR is not ready

KristinAoki · 2024-04-08T13:53:57Z

src/textbooks/textbook-card/TextbooksCard.jsx

+  const onPreviewTextbookClick = () => {
+    window.location.href = `${config.LMS_BASE_URL}/courses/${courseId}/pdfbook/${textbookIndex}/`;


I think a better user experience would be to have the preview open in a new tab.

KristinAoki · 2024-04-08T13:55:55Z

src/CourseAuthoringRoutes.jsx

@@ -120,6 +121,10 @@ const CourseAuthoringRoutes = () => {
          path="certificates"
          element={<PageWrap><Certificates courseId={courseId} /></PageWrap>}
        />
+        <Route
+          path="/textbooks"


The redirect in edx-platform needs to be updated to match the new path

Suggested change

path="/textbooks"

path="textbooks"

KristinAoki · 2024-04-08T13:58:51Z

src/generic/modal-dropzone/ModalDropzone.jsx

@@ -93,7 +102,9 @@ const ModalDropzone = ({
                  onProcessUpload={handleSelectFile}
                  inputComponent={inputComponent}
                  accept={accept}


I don't think the accept is formatted the correct way because it is not only allowing PDFs to be selected for the textbooks page. I was able to select a .docx file and was shown the invalid file type error.

KristinAoki · 2024-04-10T15:28:44Z

@vladislavkeblysh please merge in master to fix the upload to code cov causing validate/tests failure

KristinAoki

I will approve this once a PR for the textbook url redirect in edx-platform is merged in.

vladislavkeblysh · 2024-04-16T09:17:01Z

@vladislavkeblysh please merge in master to fix the upload to code cov causing validate/tests failure

Done

GlugovGrGlib · 2024-04-24T08:46:23Z

@KristinAoki The edx-platform part for textbooks was finalized already, so this PR is good to merge, thanks!

arbrandes

Looks like you committed webpack.dev-tutor.config.js by mistake. Other than that, a couple of minor requests for changes, and one big issue: it seems the URL that's being assumed here (as provided by https://github.com/openedx/edx-platform/blob/master/cms/djangoapps/contentstore/utils.py#L459) is not working. In a Tutor dev env, I must remove pages-and-resources from the path for the textbooks page to work. Any idea why that might be?

If I go to the URL manually, though, the feature works as expected.

arbrandes · 2024-04-29T19:04:27Z

src/generic/FormikControl.jsx

@@ -30,7 +30,7 @@ const FormikControl = ({
        onChange={handleChange}
        onBlur={handleBlur}
        onFocus={handleFocus}
-        isInvalid={fieldTouched && fieldError}
+        isInvalid={!!fieldTouched && !!fieldError}


Out of curiosity, why was the forced boolean conversion (!!) necessary, here?

This condition did not always work correctly, and fieldError is not a boolean value and required additional transformation

arbrandes · 2024-04-29T19:10:08Z

src/textbooks/Textbooks.jsx

+  } = useTextbooks(courseId);
+
+  const {
+    isShow: isShowProcessingNotification,


Suggested change

isShow: isShowProcessingNotification,

show: showProcessingNotification,

arbrandes · 2024-04-29T19:11:14Z

src/textbooks/Textbooks.jsx

+        </section>
+      </Container>
+      <ProcessingNotification
+        isShow={isShowProcessingNotification}


Suggested change

isShow={isShowProcessingNotification}

isShow={showProcessingNotification}

(I have a similar issue with the isShow property in <ProcessingNotification />, but that's a refactor for another day.)

arbrandes · 2024-04-29T19:15:04Z

src/textbooks/hooks.jsx

+    },
+    {
+      label: '',
+      href: `/course/${courseId}/pages-and-resources/textbooks`,


This seems to be the wrong URL. I could only get it to work (in a Tutor dev environment) with:

Suggested change

href: `/course/${courseId}/pages-and-resources/textbooks`,

href: `/course/${courseId}/textbooks`,

feat: refactor after review

GlugovGrGlib · 2024-04-30T19:18:10Z

@arbrandes The review comments were addressed, PR was rebased and ready for review.
The redirect path to textbooks page was updated in the edx-platform PR openedx/edx-platform#34668

arbrandes

Thanks for addressing my comments! Approved.

arbrandes · 2024-04-30T21:36:08Z

I went through @KristinAoki's review, and it seems all comments were addressed. So I'm going ahead and merging it. If any problems remain, we can fix them individually later.

openedx-webhooks · 2024-04-30T21:38:46Z

@vladislavkeblysh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

vladislavkeblysh requested a review from a team as a code owner March 12, 2024 19:08

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 12, 2024

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch from a0f2e15 to 57e9a55 Compare March 12, 2024 19:09

vladislavkeblysh marked this pull request as draft March 12, 2024 19:22

vladislavkeblysh marked this pull request as ready for review March 13, 2024 11:05

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch 5 times, most recently from ba30003 to e3e7c5e Compare March 13, 2024 12:16

vladislavkeblysh marked this pull request as draft March 13, 2024 16:41

vladislavkeblysh marked this pull request as ready for review March 13, 2024 16:41

vladislavkeblysh marked this pull request as draft March 13, 2024 16:42

vladislavkeblysh marked this pull request as ready for review March 13, 2024 16:44

vladislavkeblysh removed the request for review from a team March 13, 2024 16:46

vladislavkeblysh marked this pull request as draft March 13, 2024 16:48

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch from 9bf36c0 to dd34a7a Compare March 14, 2024 17:05

vladislavkeblysh changed the title ~~feat: Textbooks Page~~ feat: [FC-0044] Textbooks Page Mar 14, 2024

vladislavkeblysh requested review from abdulrafey38, arbrandes, KristinAoki and a team and removed request for abdulrafey38 and a team March 14, 2024 17:22

vladislavkeblysh self-assigned this Mar 14, 2024

PKulkoRaccoonGang requested a review from KristinAoki April 4, 2024 07:36

PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Apr 4, 2024

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch from fd3e274 to c54d011 Compare April 5, 2024 11:37

KristinAoki requested changes Apr 8, 2024

View reviewed changes

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch from e9e9e06 to 680beca Compare April 9, 2024 13:32

KristinAoki reviewed Apr 10, 2024

View reviewed changes

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch from 680beca to 1eee61b Compare April 16, 2024 09:07

GlugovGrGlib requested a review from KristinAoki April 22, 2024 07:37

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch from f8af63f to 34d8fa1 Compare April 24, 2024 08:14

arbrandes requested changes Apr 29, 2024

View reviewed changes

vladislavkeblysh force-pushed the keblysh/feat/textbooks-page branch 2 times, most recently from 23fe47e to f4f52fe Compare April 30, 2024 17:31

GlugovGrGlib mentioned this pull request Apr 30, 2024

feat: [FC-0044] Update path for MFE Textbook page openedx/edx-platform#34668

Merged

feat: textbooks page

2937a71

GlugovGrGlib force-pushed the keblysh/feat/textbooks-page branch from a5a05e9 to 7648801 Compare April 30, 2024 18:10

vladislavkeblysh added 2 commits April 30, 2024 21:19

refactor: apply fixes after 1st review round

1e7a62b

feat: refactor after review

feat: refactor after review

414ea22

feat: refactor after review

GlugovGrGlib force-pushed the keblysh/feat/textbooks-page branch from 7648801 to 6ecab8c Compare April 30, 2024 18:25

fix: remove unnecessary change to gitignore

6e5d91c

GlugovGrGlib force-pushed the keblysh/feat/textbooks-page branch from 6ecab8c to 6e5d91c Compare April 30, 2024 19:04

GlugovGrGlib requested a review from arbrandes April 30, 2024 19:18

arbrandes approved these changes Apr 30, 2024

View reviewed changes

arbrandes merged commit 65f45f7 into openedx:master Apr 30, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [FC-0044] Textbooks Page #890

feat: [FC-0044] Textbooks Page #890

vladislavkeblysh commented Mar 12, 2024 •

edited

Loading

openedx-webhooks commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

mphilbrick211 commented Mar 13, 2024

arbrandes commented Mar 13, 2024

vladislavkeblysh commented Mar 13, 2024

KristinAoki Apr 8, 2024

vladislavkeblysh Apr 9, 2024

KristinAoki Apr 8, 2024

KristinAoki Apr 8, 2024

vladislavkeblysh Apr 9, 2024

KristinAoki Apr 8, 2024

vladislavkeblysh Apr 9, 2024

KristinAoki commented Apr 10, 2024

KristinAoki left a comment

vladislavkeblysh commented Apr 16, 2024

GlugovGrGlib commented Apr 24, 2024

arbrandes left a comment

arbrandes Apr 29, 2024

vladislavkeblysh Apr 29, 2024 •

edited

Loading

arbrandes Apr 29, 2024

vladislavkeblysh Apr 30, 2024

arbrandes Apr 29, 2024

vladislavkeblysh Apr 30, 2024

arbrandes Apr 29, 2024

GlugovGrGlib commented Apr 30, 2024

arbrandes left a comment

arbrandes commented Apr 30, 2024

openedx-webhooks commented Apr 30, 2024

		const onPreviewTextbookClick = () => {
		window.location.href = `${config.LMS_BASE_URL}/courses/${courseId}/pdfbook/${textbookIndex}/`;

	isShow: isShowProcessingNotification,
	show: showProcessingNotification,

	isShow={isShowProcessingNotification}
	isShow={showProcessingNotification}

	href: `/course/${courseId}/pages-and-resources/textbooks`,
	href: `/course/${courseId}/textbooks`,

feat: [FC-0044] Textbooks Page #890

feat: [FC-0044] Textbooks Page #890

Conversation

vladislavkeblysh commented Mar 12, 2024 • edited Loading

openedx-webhooks commented Mar 12, 2024 • edited Loading

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

mphilbrick211 commented Mar 13, 2024

arbrandes commented Mar 13, 2024

vladislavkeblysh commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristinAoki commented Apr 10, 2024

KristinAoki left a comment

Choose a reason for hiding this comment

vladislavkeblysh commented Apr 16, 2024

GlugovGrGlib commented Apr 24, 2024

arbrandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladislavkeblysh Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlugovGrGlib commented Apr 30, 2024

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes commented Apr 30, 2024

openedx-webhooks commented Apr 30, 2024

vladislavkeblysh commented Mar 12, 2024 •

edited

Loading

openedx-webhooks commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

vladislavkeblysh Apr 29, 2024 •

edited

Loading