Skip to content

feat: add plan_only param to api and create /tags/import/plan endpoint [FC-0036]#130

Merged
bradenmacdonald merged 5 commits intoopenedx:mainfrom
open-craft:rpenido/fal-3539-add_plan_only_api_and_endpoint
Dec 19, 2023
Merged

feat: add plan_only param to api and create /tags/import/plan endpoint [FC-0036]#130
bradenmacdonald merged 5 commits intoopenedx:mainfrom
open-craft:rpenido/fal-3539-add_plan_only_api_and_endpoint

Conversation

@rpenido
Copy link
Contributor

@rpenido rpenido commented Dec 6, 2023 •
edited
Loading

Description

This PR adds a plan_only param to the API and creates a new REST endpoint to allow the frontend to query for an import plan without actually importing anything.

More info

Part of:

Testing instructions

  • Please ensure that the tests cover the expected behavior

Private-ref:

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 6, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Dec 6, 2023

Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@rpenido rpenido force-pushed the rpenido/fal-3539-add_plan_only_api_and_endpoint branch 2 times, most recently from e0b37ae to 1545e37 Compare December 6, 2023 22:27
@rpenido rpenido force-pushed the rpenido/fal-3539-add_plan_only_api_and_endpoint branch from 1545e37 to 7f953d8 Compare December 6, 2023 22:51
ChrisChV
ChrisChV approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@ChrisChV ChrisChV left a comment

Choose a reason for hiding this comment

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

@rpenido Looks good 👍 When you fix the nits, will be ready for CC review

  • I tested this: I read and check the tests
  • I read through the code and considered the security, stability and performance implications of the changes.
  • Includes tests for bugfixes and/or features added.
  • Includes documentation

tests/openedx_tagging/core/tagging/test_views.py Outdated
Comment on lines 2428 to 2434
# Check if the taxonomy was not changed
url = TAXONOMY_TAGS_URL.format(pk=self.taxonomy.id)
response = self.client.get(url)
tags = response.data["results"]
assert len(tags) == len(self.old_tags)
for i, tag in enumerate(tags):
assert tag["value"] == self.old_tags[i].value
Copy link
Contributor

@ChrisChV ChrisChV Dec 7, 2023

Choose a reason for hiding this comment

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

Could you add this on a separate utility function? It is repeated in different tests.

Copy link
Contributor Author

@rpenido rpenido Dec 8, 2023

Choose a reason for hiding this comment

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

Thank you @ChrisChV! The code grew, and I overlooked that.
Done fc3a6da!

tests/openedx_tagging/core/tagging/test_views.py
{"file": file},
format="multipart"
)
assert response.status_code == status.HTTP_200_OK
Copy link
Contributor

@ChrisChV ChrisChV Dec 7, 2023

Choose a reason for hiding this comment

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

@rpenido Is it possible to do more verifications here? Maybe verify that it does not return error messages and that it does return data

Copy link
Contributor Author

@rpenido rpenido Dec 8, 2023

Choose a reason for hiding this comment

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

Done e0fe6e8!

@rpenido rpenido mentioned this pull request Dec 11, 2023
Closed
@rpenido
Copy link
Contributor Author

rpenido commented Dec 19, 2023

Hi @bradenmacdonald ! This is ready for CC review.

@rpenido rpenido mentioned this pull request Dec 19, 2023
Merged
bradenmacdonald
bradenmacdonald approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@bradenmacdonald bradenmacdonald merged commit 1d9c459 into openedx:main Dec 19, 2023
@bradenmacdonald bradenmacdonald deleted the rpenido/fal-3539-add_plan_only_api_and_endpoint branch December 19, 2023 21:07
@openedx-webhooks
Copy link

openedx-webhooks commented Dec 19, 2023

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@rpenido rpenido mentioned this pull request Dec 20, 2023
Merged
@ormsbee ormsbee mentioned this pull request Mar 24, 2025
Closed
@ormsbee ormsbee mentioned this pull request Sep 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bradenmacdonald bradenmacdonald bradenmacdonald approved these changes

@ChrisChV ChrisChV ChrisChV approved these changes

Assignees

No one assigned

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rpenido @openedx-webhooks @bradenmacdonald @ChrisChV