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.

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 90.98361% with 11 lines in your changes missing coverage. Please review.

Project coverage is 93.20%. Comparing base (98fbcff) to head (2a96990).
Report is 9 commits behind head on master.

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.23%   93.20%   -0.04%     
==========================================
  Files        1098     1100       +2     
  Lines       21709    21887     +178     
  Branches     4593     4744     +151     
==========================================
+ Hits        20240    20399     +159     
- Misses       1404     1414      +10     
- 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.

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 from e995900 to 8c23e0d Compare January 14, 2025 06:45

dcoa added 8 commits January 16, 2025 08:21

refactor: block creation for text component in librries v2

89efcfc

refactor: allow init the modal for video creation

dc786ec

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

658a4b0

fix: clean the editor store when the moda is close

7d20422

test: add testing for request

65174e0

test: testing for editor container hook

778a854

test: refactor editor testing

30c2f11

feat: implement fist versioning of image upload for creation workflow

e677ed0

dcoa force-pushed the dcoa/editor-create-block branch from 8c23e0d to e677ed0 Compare January 16, 2025 03:42

dcoa added 2 commits January 17, 2025 18:10

feat: allow to update images in the creation workflow

bf9c8cc

fix: create problem with unique id

ceb9f39

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

test: refactor add content library test

3dd53d4

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

feat: add error message in editor create block

a8f286c

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

test: update testing

82e5d80

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 from 3ac5427 to f350757 Compare January 29, 2025 03:02

refactor: change isCreateBlock for shouldCreateBlock

9a547a6

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

fix: keep collection linkComponent function when the modal is close

b3d31d7

dcoa force-pushed the dcoa/editor-create-block branch from f4d678b to c67b88d Compare January 30, 2025 09:53

test: increse coverage for EditorContainer component

cfae82d

dcoa force-pushed the dcoa/editor-create-block branch 3 times, most recently from 5f19a99 to bb57bd4 Compare February 3, 2025 06:13

refactor: modify create unique block key

2a96990

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

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

Loading

openedx-webhooks commented Dec 17, 2024 •

edited

Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

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

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 Loading

Description

Supporting information

Testing instructions

Other information

openedx-webhooks commented Dec 17, 2024 • edited Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

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

bradenmacdonald Jan 27, 2025

Choose a reason for hiding this comment

bradenmacdonald Jan 27, 2025

Choose a reason for hiding this comment

dcoa Jan 29, 2025

Choose a reason for hiding this comment

bradenmacdonald Jan 27, 2025

Choose a reason for hiding this comment

dcoa Jan 29, 2025

Choose a reason for hiding this comment

bradenmacdonald Jan 27, 2025

Choose a reason for hiding this comment

dcoa Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald Jan 30, 2025

Choose a reason for hiding this comment

bradenmacdonald commented Jan 30, 2025

dcoa commented Dec 17, 2024 •

edited

Loading

openedx-webhooks commented Dec 17, 2024 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

dcoa Jan 29, 2025 •

edited

Loading