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

feat: Add export_id field on Taxonomy #145

Merged

Conversation

ChrisChV
Copy link
Contributor

@ChrisChV ChrisChV commented Jan 26, 2024

Description

This PR adds a new export_id field on Taxonomy model.

Support information

Testing instruction

  • Verify that all tests pass and that they cover the requirements.

Before run migration

  • Start a server with python manage.py runsever.
  • Create a superuser with python manage.py createsuperuser.
  • Go to Taxonomy list on Django Admin.
  • Create two Taxonomies with the same name.
  • Run the migrations: python manage.py migrate.
  • Verify that the migrations have been executen without errors.
  • On Django admin create a new Taxonomy and verify that you need to add an export_id
  • Verify that you can edit the export_id on Django admin.
  • Verify the validations of the export_id

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 26, 2024
@openedx-webhooks
Copy link

Thanks for the pull request, @ChrisChV! 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.

@@ -121,7 +121,7 @@ def import_tags(
task.end_success()

return True, task, tag_import_plan
except Exception as exception: # pylint: disable=broad-exception-caught
except Exception as exception:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This generated a warning on the lint

Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

Couple of nits/questions, but everything else looks perfect. 👍

@ChrisChV Let me know if you need me to do a second review?

  • I tested this in my devstack using the PR test instructions
  • I read through the code
  • I checked for accessibility issues N/A here
  • Includes documentation -- adds help_text for new field, docstrings where needed
  • User-facing strings are extracted for translation

openedx_tagging/core/tagging/rest_api/v1/views.py Outdated Show resolved Hide resolved
openedx_tagging/core/tagging/rest_api/v1/views.py Outdated Show resolved Hide resolved
tests/openedx_tagging/core/tagging/test_api.py Outdated Show resolved Hide resolved
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.

LGTM!

@pomegranited pomegranited merged commit a655224 into openedx:main Feb 1, 2024
7 checks passed
@openedx-webhooks
Copy link

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

@pomegranited pomegranited deleted the chris/FAL-3620-export-id-on-taxonomies branch February 1, 2024 23:48
@ChrisChV ChrisChV restored the chris/FAL-3620-export-id-on-taxonomies branch February 13, 2024 16:45
@ChrisChV ChrisChV deleted the chris/FAL-3620-export-id-on-taxonomies branch February 13, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants