refactor: block creation in libraries v2 #1574

dcoa · 2024-12-17T05:10:46Z

Description

This PR changes the way components are creating in libraries V2 with the purpose to avoid the creation of a blank component as fist step, behavior that have the following problems:

The "cancel" action doesn't cancel the addition of the component, which seems like a bug.
We can't set a Give components "friendly" slugs on creation #1384 for the component, because its title is not set at the time of creation.

Supporting information

This PR is liked to #1482 (comment) issue.

Testing instructions

Create a component in libraries V2 supported by the editor [text, video, problem]
Press the cancel button or the close icon, no new component should appear in the platform.
Prove the right creating workflow:
- Update the title and save it, should be used as usage_key block.
- If the title does not change should create a unique key.
- The user is enable to update images.
- Press the save button should create the component and save the content.

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.

PR sandbox

UN/PW: openedx / openedx

Settings

openedx-webhooks · 2024-12-17T05:10:51Z

Thanks for the pull request, @dcoa!

This repository is currently maintained by @openedx/2u-tnl.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If 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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

kdmccormick · 2025-01-10T17:25:02Z

Hey @dcoa , I'm interested in making sure this gets across the line. Is there anything blocking you or anything that you'd like a second set of eyes on?

bradenmacdonald · 2025-01-10T17:30:41Z

It just occurred to me that if we need a place to store uploaded static assets when they're uploaded before saving, we could potentially use StagedContent to hold the draft version of the new component. Not sure if it's a good idea or if we should just save things locally in the browser, which is another alternative.

dcoa · 2025-01-13T10:23:10Z

Sorry, I have had some issues with the pc and additional tasks that slow my progress, but I will continue working on it tomorrow, most of the code is already functional but I need to change the reset logic a little bit.

Thanks @bradenmacdonald for your help giving me options for the images issue, I will start testing that.

codecov · 2025-01-21T09:27:23Z

Codecov Report

Attention: Patch coverage is 91.12903% with 11 lines in your changes missing coverage. Please review.

Project coverage is 93.20%. Comparing base (b0fc3d9) to head (f174570).

Files with missing lines	Patch %	Lines
src/editors/data/redux/thunkActions/requests.js	65.38%	8 Missing and 1 partial ⚠️
src/editors/data/redux/thunkActions/app.js	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1574      +/-   ##
==========================================
- Coverage   93.24%   93.20%   -0.05%     
==========================================
  Files        1101     1101              
  Lines       21859    21943      +84     
  Branches     4643     4743     +100     
==========================================
+ Hits        20383    20452      +69     
- Misses       1411     1417       +6     
- Partials       65       74       +9

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

dcoa · 2025-01-27T21:14:51Z

@kdmccormick @bradenmacdonald hi, I think the PR is good to start the review process (I just need to fix some test coverage - but the functionality is already implemented and working)

bradenmacdonald

Very nice work @dcoa ! This is exciting to see and will be a really big improvement.

.env.test

src/editors/data/redux/app/selectors.ts

bradenmacdonald · 2025-01-27T23:07:12Z

src/editors/data/redux/index.ts

  app: app.reducer,
  requests: requests.reducer,
  video: video.reducer,
  problem: problem.reducer,
  game: game.reducer,
 });

+const rootReducer = (state: any, action: any) => {
+  if (action.type === 'resetEditor') {


I'm kind of surprised we didn't have this resetEditor before.

src/editors/data/redux/thunkActions/app.js

bradenmacdonald · 2025-01-27T23:16:16Z

src/editors/data/redux/thunkActions/requests.js

+    const problemTitles = new Set([...Object.values(ProblemTypes).map((problem) => problem.title),
+      ...Object.values(AdvanceProblems).map((problem) => problem.title)]);
+
+    definitionId = problemTitles.has(blockTitle) ? `${uuid4()}` : definitionId;


Can you please add a comment to explain what this code is doing? It's not clear to me what it's for, and why it sometimes needs a UUID as the definition ID.

The problem editor has 2 steps, fist you need to select the type of problem and secondly edit/add the content. When the problem type is selected that becomes the block title, if the user don't change it will create the UUID to avoid create blocks with the default title and eventually avoid the definitionId is already taken problem.

bradenmacdonald · 2025-01-27T23:17:01Z

src/editors/data/redux/thunkActions/requests.js

+    promise: createLibraryBlock({
+      libraryId: selectors.app.learningContextId(getState()),
+      blockType: selectors.app.blockType(getState()),
+      definitionId,
+    }),


Should we throw an error if the learning context is not a library, but we get to this function? Or is that definitely impossible?

At the moment only libraries launch the creation action (I didn't modify courses workflow so it could be manage in a different PR and avoid make something too large and hard for reviewing)

bradenmacdonald · 2025-01-27T23:17:26Z

src/editors/data/redux/thunkActions/requests.js

+  const blockTitle = selectors.app.blockTitle(getState());
+  let definitionId = blockTitle
+    ? blockTitle.toLowerCase().replaceAll(' ', '-')
+    : `${uuid4()}`;


This can happen in a follow-up PR, but I think we need a better way to handle what happens when this definitionId is already taken. It should automatically generate a unique ID by appending a number to the definitionId, but instead it just throws an error:

This error could be confusing for users since it's based on the title, but titles aren't required to be unique.

I used the title based on my understanding of #1384 (comment), a possible fix could be add a shorten hash after the title part.

a possible fix could be add a shorten hash after the title part.

That makes sense to me, yep.

src/library-authoring/add-content/AddContentContainer.test.tsx

src/library-authoring/add-content/AddContentContainer.tsx

bradenmacdonald · 2025-01-30T01:16:13Z

Thanks for those changes! I'll test this again tomorrow :) I've also asked if anyone else wants to review it.

dcoa · 2025-02-05T00:42:18Z

src/editors/data/redux/thunkActions/requests.js

+    .then(() => new Promise((resolve) => {
+      dispatch(module.uploadAsset({
+        asset,
+        onSuccess: (response) => {


I'm facing a challenge to cover those lines in the test, the promise chain is executed but I couldn't see the onSuccess in upload assets launch. I tried to reorder the test but not successfully, do you have a suggestion @bradenmacdonald ?

Sorry for the delay @dcoa. I'm testing this PR and I will try to see if I have any suggestions about this.

@dcoa I am not a fan of this kind of test case where so many things are mocked out. I think that's the root problem.

In any case, from what I can see, the batchUploadAssets test cases in src/editors/data/redux/thunkActions/requests.test.js are executing the start of the promise chain, so that

frontend-app-authoring/src/editors/data/redux/thunkActions/requests.js

Line 164 in 2a96990

dispatch(module.uploadAsset({

is called. However, in this whole test suite, uploadAsset is mocked out, so it doesn't do anything. It never calls onSuccess, and so the promise chain never completes - only the first part of it is partially executed. The fix is to replace the mocked version of uploadAsset with one that actually does call its onSuccess argument, but I'm not sure how to do that.

kdmccormick · 2025-02-05T18:39:18Z

I will try to leave a review this week, but if you feel confident in the change and I haven't reviewed yet, don't block merging on me :)

bradenmacdonald · 2025-02-06T18:45:46Z

src/editors/data/redux/thunkActions/requests.js

+    // add a hash to prevent duplication.
+    const hash = uuid4().split('-')[4];
+    definitionId = `${cleanTitle.replaceAll(/\s+/g, '-')}-${hash}`;


Suggested change

// add a hash to prevent duplication.

const hash = uuid4().split('-')[4];

definitionId = `${cleanTitle.replaceAll(/\s+/g, '-')}-${hash}`;

// add a short random suffix to prevent conflicting IDs.

const suffix = uuid4().split('-')[4];

definitionId = `${cleanTitle.replaceAll(/\s+/g, '-')}-${suffix}`;

bradenmacdonald · 2025-02-06T18:57:37Z

src/editors/data/redux/thunkActions/requests.js

+    .then(() => new Promise((resolve) => {
+      dispatch(module.uploadAsset({
+        asset,
+        onSuccess: (response) => {


Sorry for the delay @dcoa. I'm testing this PR and I will try to see if I have any suggestions about this.

bradenmacdonald · 2025-02-07T23:09:23Z

src/library-authoring/add-content/AddContentContainer.tsx

+    const suportedEditorTypes = Object.values(blockTypes);
+    if (suportedEditorTypes.includes(blockType)) {
+      // linkComponent on editor close.
+      openComponentEditor('', (data) => linkComponent(data.id), blockType);


If I create a new "Text" component and then cancel without saving changes, this is giving an error:

Cannot read properties of undefined (reading 'id')

open-craft-grove · 2025-02-10T19:06:25Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2025-02-12T04:51:26Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2025-02-12T05:44:46Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

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

dcoa changed the title ~~refactor: block creation~~ refactor: block creation in libraries v2 Dec 17, 2024

dcoa force-pushed the dcoa/editor-create-block branch 4 times, most recently from 94b0493 to 6af6ee7 Compare December 18, 2024 05:25

dcoa force-pushed the dcoa/editor-create-block branch from f70d770 to b8ecbe2 Compare December 27, 2024 00:24

dcoa force-pushed the dcoa/editor-create-block branch 4 times, most recently from 871b242 to 4832895 Compare January 21, 2025 05:20

dcoa force-pushed the dcoa/editor-create-block branch from 4832895 to 3dd53d4 Compare January 21, 2025 09:12

dcoa force-pushed the dcoa/editor-create-block branch from 1773448 to a8f286c Compare January 23, 2025 06:52

dcoa marked this pull request as ready for review January 24, 2025 04:12

dcoa requested a review from a team as a code owner January 24, 2025 04:12

dcoa requested review from bradenmacdonald and removed request for a team January 24, 2025 04:12

dcoa force-pushed the dcoa/editor-create-block branch from fc1b4cf to 82e5d80 Compare January 24, 2025 05:04

bradenmacdonald requested changes Jan 28, 2025

View reviewed changes

dcoa force-pushed the dcoa/editor-create-block branch 2 times, most recently from f350757 to 9a547a6 Compare January 29, 2025 03:11

dcoa force-pushed the dcoa/editor-create-block branch 2 times, most recently from c67b88d to b9b4329 Compare February 3, 2025 04:54

dcoa force-pushed the dcoa/editor-create-block branch from bb57bd4 to 2a96990 Compare February 3, 2025 06:36

dcoa commented Feb 5, 2025

View reviewed changes

kdmccormick self-requested a review February 5, 2025 18:38

bradenmacdonald reviewed Feb 6, 2025

View reviewed changes

bradenmacdonald reviewed Feb 7, 2025

View reviewed changes

dcoa added 13 commits February 10, 2025 11:26

refactor: block creation for text component in librries v2

c5baa98

refactor: allow init the modal for video creation

b41ae55

refactor: allow open the problem modal when is a new component

322dbea

fix: clean the editor store when the moda is close

db5fe3f

test: add testing for request

84f0f48

test: testing for editor container hook

c744219

test: refactor editor testing

cac6446

feat: implement fist versioning of image upload for creation workflow

b36830c

feat: allow to update images in the creation workflow

d46a649

fix: create problem with unique id

f9f66ad

test: refactor add content library test

08dce9d

feat: add error message in editor create block

37824d9

test: update testing

4879c60

dcoa force-pushed the dcoa/editor-create-block branch from 9b2be97 to d6c0a0c Compare February 10, 2025 01:37

dcoa added 5 commits February 10, 2025 12:58

refactor: change isCreateBlock for shouldCreateBlock

4243c46

fix: keep collection linkComponent function when the modal is close

3b10f14

test: increse coverage for EditorContainer component

737cc8e

refactor: modify create unique block key

f90a498

fix: validate data for onClose function

f174570

dcoa force-pushed the dcoa/editor-create-block branch from d6c0a0c to f174570 Compare February 10, 2025 01:59

kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Feb 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: block creation in libraries v2 #1574

refactor: block creation in libraries v2 #1574

dcoa commented Dec 17, 2024 •

edited by kdmccormick

Loading

openedx-webhooks commented Dec 17, 2024 •

edited

Loading

kdmccormick commented Jan 10, 2025

bradenmacdonald commented Jan 10, 2025

dcoa commented Jan 13, 2025

codecov bot commented Jan 21, 2025 •

edited

Loading

dcoa commented Jan 27, 2025

bradenmacdonald left a comment

bradenmacdonald Jan 27, 2025

bradenmacdonald Jan 27, 2025

dcoa Jan 29, 2025

bradenmacdonald Jan 27, 2025

dcoa Jan 29, 2025

bradenmacdonald Jan 27, 2025

dcoa Jan 29, 2025 •

edited

Loading

bradenmacdonald Jan 30, 2025

bradenmacdonald commented Jan 30, 2025

dcoa Feb 5, 2025

bradenmacdonald Feb 6, 2025

bradenmacdonald Feb 8, 2025

kdmccormick commented Feb 5, 2025

bradenmacdonald Feb 6, 2025

bradenmacdonald Feb 6, 2025

bradenmacdonald Feb 7, 2025

open-craft-grove commented Feb 10, 2025

open-craft-grove commented Feb 12, 2025

open-craft-grove commented Feb 12, 2025

refactor: block creation in libraries v2 #1574

Are you sure you want to change the base?

refactor: block creation in libraries v2 #1574

Conversation

dcoa commented Dec 17, 2024 • edited by kdmccormick Loading

Description

Supporting information

Testing instructions

Other information

PR sandbox

openedx-webhooks commented Dec 17, 2024 • edited Loading

kdmccormick commented Jan 10, 2025

bradenmacdonald commented Jan 10, 2025

dcoa commented Jan 13, 2025

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

dcoa commented Jan 27, 2025

bradenmacdonald left a 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

Choose a reason for hiding this comment

dcoa Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

open-craft-grove commented Feb 10, 2025

open-craft-grove commented Feb 12, 2025

open-craft-grove commented Feb 12, 2025

dcoa commented Dec 17, 2024 •

edited by kdmccormick

Loading

openedx-webhooks commented Dec 17, 2024 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

dcoa Jan 29, 2025 •

edited

Loading