Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: test only accepts int values in priority #12

Closed
wants to merge 4 commits into from

Conversation

johanseto
Copy link
Collaborator

@johanseto johanseto commented Jan 29, 2024

Description

This fix is a test for this feature #2.

This test https://github.com/openedx/edx-platform/blob/open-release/palm.master/cms/djangoapps/contentstore/rest_api/v0/tests/test_tabs.py#L108 dont accept None value in a list sort function.

So as this is order ascending, a big value gives the same behavior

Testing instructions

Check the tabs and they continue ordering in the desired way as the proposed in this PR #2 .
2024-01-29_16-37

Before

Tests was failing in static tabs.
https://github.com/nelc/edx-platform/actions/runs/7702081514/job/20990641623?pr=11
2024-01-29_16-32

Other information

PRs related
#2
eduNEXT/edunext-platform#678
eduNEXT/edunext-platform#801

@johanseto johanseto force-pushed the jlc/fix-statics-tabs-tests branch from a30da94 to 879ad70 Compare January 29, 2024 22:15
This test https://github.com/openedx/edx-platform/blob/open-release/palm.master/cms/djangoapps/contentstore/rest_api/v0/tests/test_tabs.py#L108
dont accept None value in a list sort function.

So as this is order ascending, a big value gives the same beahviour
Also progress would be first than dates.
@johanseto johanseto force-pushed the jlc/fix-statics-tabs-tests branch 2 times, most recently from a25cdf0 to 942671c Compare January 29, 2024 23:45
now dates is the last element of ithe list and progress is always before dates.(penultimate)
@johanseto johanseto force-pushed the jlc/fix-statics-tabs-tests branch from 942671c to 61816d5 Compare January 29, 2024 23:46
@johanseto
Copy link
Collaborator Author

johanseto commented Jan 30, 2024

The commit 61816d5 fixes the deletion of the progress tab because now is at the end of the list.
2024-01-29_18-26
2024-01-29_18-23

@johanseto johanseto force-pushed the jlc/fix-statics-tabs-tests branch from 5ccf60c to 6109a1c Compare January 30, 2024 00:58
Copy link
Collaborator

@andrey-canon andrey-canon left a comment

Choose a reason for hiding this comment

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

As this pr mentioned these changes were required to fix the tabs sorting however the pr#2 doesn't make that, that just moves at the end the wiki, progress and syllabus tabs but it's not possible to change the order of the tabs

maple e.g
maple-tabs

palm without mfe
image

the mfe version is more limited
image

Basically that functionality was removed in this pr when they changed the is_movable attribute default value
image

So my advice is to close this pr, revert the pr #2 and include this change in this list since that sorting feature was deprecated and our changes doesn't cover the necessity that we had

@johanseto
Copy link
Collaborator Author

johanseto commented Jan 31, 2024

As this pr mentioned these changes were required to fix the tabs sorting however the pr#2 doesn't make that, that just moves at the end the wiki, progress and syllabus tabs but it's not possible to change the order of the tabs

maple e.g maple-tabs maple-tabs

palm without mfe image

the mfe version is more limited image

Basically that functionality was removed in this pr when they changed the is_movable attribute default value image

So my advice is to close this pr, revert the pr #2 and include this change in this list since that sorting feature was deprecated and our changes doesn't cover the necessity that we had

According to that, this gonna be closed in favor of #13

@johanseto johanseto closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants