Skip to content

remove autofilled title value from contentNode#2675

Merged
rtibbles merged 2 commits intolearningequality:developfrom
marcellamaki:remove-title-autofill-in-topic
Jan 15, 2021
Merged

remove autofilled title value from contentNode#2675
rtibbles merged 2 commits intolearningequality:developfrom
marcellamaki:remove-title-autofill-in-topic

Conversation

@marcellamaki
Copy link
Copy Markdown
Member

Addresses #2565

Before/After Screenshots (if applicable)

Before:
subtopic-1

After:
subtopic-2

Steps to Test

  • Open a channel and create a subtopic using the options menu in the side panel.
  • Form should open without a title autofilled and if required field are completed, should save without error.

Copy link
Copy Markdown
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

I saw the related discussion and it makes sense to remove automatically filled title. Though it doesn't seem to be the primary cause of issues described in #2565.

The following seems to be happening:

  1. createContentNode action is called with an auto filled title value in its payload
  2. This payload gets merged after the default content node data here but complete field stays set to default false value even though there might be data in the payload that make the new node valid.

Although this PR helps, I assume that it's because when the topic title is left empty, complete should indeed be false because that's not a valid state for a topic, so it's not a problem that default false value for complete is used. When someone updates the topic later, updateContentNode is called and in it, complete field is updated.

I'd suggest fixing this issue in createContentNode so that we can be sure that validation is up to date whenever we call this action. Doing something like:

const contentNodeData = {
    title: '',
    description: '',
    kind,
    tags: {},
    extra_fields: {},
    [NEW_OBJECT]: true,
    changed: true,
    language: session.preferences ? session.preferences.language : session.currentLanguage,
    parent,
    ...contentDefaults,
    role_visibility: contentDefaults.role_visibility || RolesNames.LEARNER,
    ...payload,
  };

 contentNodeData.complete = isNodeComplete({ nodeDetails: contentNodeData, assessmentItems: [], files: [] })

should help.

@micahscopes micahscopes self-requested a review January 7, 2021 22:19
micahscopes
micahscopes previously approved these changes Jan 7, 2021
Copy link
Copy Markdown
Contributor

@micahscopes micahscopes left a comment

Choose a reason for hiding this comment

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

Approved!

@micahscopes micahscopes dismissed their stale review January 7, 2021 22:21

I didn't read the requested changes closely enough

@micahscopes micahscopes self-requested a review January 7, 2021 22:23
@micahscopes
Copy link
Copy Markdown
Contributor

@marcellamaki and I took a look at this today and found some funny stuff going on that might point to other underlying issues. Since it's not super high stakes, we decided it'd be best to leave this as is until @MisRob gets a chance to take a look.

@marcellamaki
Copy link
Copy Markdown
Member Author

Since the UX part of the issue that the PR addresses is no longer reproducible on develop, I don't think these changes need to be merged prior to Studio relaunch. I am going to pause on this PR and put back as a draft/WIP, and reconnect with @MisRob to review her suggestions/comments about potential underlying improvements that could be made and decide how to proceed after that.

@marcellamaki marcellamaki marked this pull request as draft January 8, 2021 18:25
@marcellamaki marcellamaki marked this pull request as ready for review January 14, 2021 16:07
@marcellamaki marcellamaki requested a review from MisRob January 14, 2021 16:07
@marcellamaki marcellamaki removed the request for review from micahscopes January 14, 2021 16:08
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 14, 2021

Codecov Report

Merging #2675 (b86ce9d) into develop (a81e7d4) will decrease coverage by 0.20%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2675      +/-   ##
===========================================
- Coverage    85.14%   84.94%   -0.21%     
===========================================
  Files          299      309      +10     
  Lines        15077    15367     +290     
===========================================
+ Hits         12838    13054     +216     
- Misses        2239     2313      +74     
Impacted Files Coverage Δ
contentcuration/contentcuration/views/nodes.py 68.75% <0.00%> (-19.96%) ⬇️
contentcuration/contentcuration/tasks.py 62.58% <0.00%> (-12.18%) ⬇️
contentcuration/contentcuration/views/base.py 58.51% <0.00%> (-4.30%) ⬇️
contentcuration/contentcuration/api.py 92.06% <0.00%> (-2.51%) ⬇️
...ntentcuration/contentcuration/utils/gcs_storage.py 73.46% <0.00%> (-1.80%) ⬇️
.../contentcuration/tests/viewsets/test_invitation.py 89.47% <0.00%> (-1.49%) ⬇️
contentcuration/contentcuration/models.py 85.52% <0.00%> (-0.25%) ⬇️
...uration/contentcuration/tests/test_contentnodes.py 94.60% <0.00%> (-0.20%) ⬇️
contentcuration/search/viewsets/contentnode.py 52.99% <0.00%> (-0.06%) ⬇️
contentcuration/contentcuration/tests/testdata.py 93.75% <0.00%> (-0.05%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6b0135...b86ce9d. Read the comment docs.

@rtibbles rtibbles merged commit 92b4aa9 into learningequality:develop Jan 15, 2021
Sign up for free to 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.

Adding new subtopic shows node as invalid even though there aren't any issues

4 participants