Skip to content

Fixes folders or resources with missing title remain 'incomplete' after editing just the title - #5539

Merged
akolson merged 5 commits into
learningequality:hotfixesfrom
AllanOXDi:fixes-resources-remaining-incomplete-after-editing
Nov 25, 2025
Merged

Fixes folders or resources with missing title remain 'incomplete' after editing just the title#5539
akolson merged 5 commits into
learningequality:hotfixesfrom
AllanOXDi:fixes-resources-remaining-incomplete-after-editing

Conversation

@AllanOXDi

Copy link
Copy Markdown
Contributor

Summary

This PR fixes folders or resources with missing title remain 'incomplete' after editing just the title
Closes#5347

References

#5347

Before

2025-09-01_17-24-52.mp4

After

Screen.Recording.2025-11-06.at.18.45.55.mov

Reviewer guidance

  1. Sign in to Studio
  2. Go to a channel and add a folder or a resource without a title
  3. Edit just the title of the folder or resource and observe that it's still marked as incomplete

@akolsonakolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AllanOXDi! I left a general comment about the implementation. Thanks

@akolsonakolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks correct to me! Manual QA also checks out. Thanks @AllanOXDi

@akolson

Copy link
Copy Markdown
Member

@AllanOXDi can we re-target this to hotfixes? Thanks

@AllanOXDi
AllanOXDi changed the base branch from unstable to hotfixesNovember 20, 2025 12:49
@AllanOXDi
AllanOXDi changed the base branch from hotfixes to unstableNovember 20, 2025 12:49
@AllanOXDi
AllanOXDiforce-pushed the fixes-resources-remaining-incomplete-after-editing branch from 590f5ea to 0047ecaCompareNovember 20, 2025 13:04
@AllanOXDi
AllanOXDi changed the base branch from unstable to hotfixesNovember 20, 2025 13:09
@AllanOXDi
AllanOXDi changed the base branch from hotfixes to unstableNovember 20, 2025 13:59
@AllanOXDi
AllanOXDi changed the base branch from unstable to hotfixesNovember 20, 2025 14:00
@akolson

Copy link
Copy Markdown
Member

@AllanOXDi, !t looks like the branch is from unstable so we might have to squash all the commits from unstable. I think cherypicking the commit instead should do the trick? or rebasing?

@AllanOXDi

Copy link
Copy Markdown
ContributorAuthor

working on that

@AllanOXDi
AllanOXDiforce-pushed the fixes-resources-remaining-incomplete-after-editing branch from 0047eca to 2378808CompareNovember 20, 2025 16:59
@AllanOXDi

Copy link
Copy Markdown
ContributorAuthor

Fixed!

Comment threadrequirements.txt Outdated

@akolsonakolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a blocking comment. All else looks correct to me!

@akolsonakolson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All should be good now. Thanks @AllanOXDi

General comment on the tests updates: I wonder if making the tests more liberal using the objectContaining function could increase the risk of masking bugs as it only asserts that certain keys exist, not that the whole payload is exactly what is expected (which I think is the intention of the previous test). Not a blocker but something to think about

@AllanOXDi

Copy link
Copy Markdown
ContributorAuthor

Totally fair point ! objectContaining is more permissive.
In this case though, that’s intentional: the test’s job is just to ensure the important fields (id, title, description ) are sent correctly, not to lock down the entire payload shape. Using objectContaining keeps the test focused on behavior rather than structure, and avoids breaking anytime we add non-essential fields. So it’s still a safe assertion for the contract we care about here?

@akolson

Copy link
Copy Markdown
Member

I agree! However, the intent of the tests is lost--we've added the checkComplete to the payload in the updateContentNode action, so using objectContaining would bypass the added field. I think the test failures could have been resolved by just updating the tests to have the checkComplete: true,. I suspect the two tests are failing because they default checkComplete to undefined thus the difference in the expected payload.

expect(updateContentNode).toHaveBeenCalledWith({id: nodeId,title: newTitle,description: newDescription// or '',checkComplete: true,});

@AllanOXDi

Copy link
Copy Markdown
ContributorAuthor

Updated! I leaned toward using objectContaininghis because I see that this modal isn’t the one adding `checkComplete that happens inside the action logic.

@akolson

Copy link
Copy Markdown
Member

Correct! It however makes use of it while calling the updateContentNode function.

Thanks @AllanOXDi for the fix. Will merge this

@akolson
akolson merged commit 76ae31b into learningequality:hotfixesNov 25, 2025
13 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Folders or resources with missing title remain 'incomplete' after editing just the title

2 participants

@AllanOXDi@akolson