Skip to content

Conversation

@rpenido
Copy link
Contributor

@rpenido rpenido commented Jul 10, 2023

Description

This PR adds support for creating, viewing, updating and deleting Taxonomies via REST APIs, with the permissions applied accordingly.

Supporting Information

Testing instructions

  • Ensure that all endpoints in the related issue are implemented.
  • Ensure that the tests cover the expected behaviour of the view and permissions as described in the related issue.

To do before merge

  • squash and merge this change (don't just merge)

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

openedx-webhooks commented Jul 10, 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.

@pomegranited

This comment was marked as outdated.

@rpenido rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch 9 times, most recently from adfcbbd to 7c93cd2 Compare July 21, 2023 18:40
@rpenido rpenido marked this pull request as ready for review July 21, 2023 19:01
@rpenido
Copy link
Contributor Author

rpenido commented Jul 21, 2023

@pomegranited Updated this PR. I think we are ready to review here!

@rpenido rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from 2b6fd16 to 1b1b59a Compare July 21, 2023 19:04
@rpenido rpenido changed the title Taxonomy view/management APIs [WIP] Taxonomy view/management APIs Jul 21, 2023
@pomegranited pomegranited changed the title Taxonomy view/management APIs Taxonomy view/management REST APIs Jul 24, 2023
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.

Hi @rpenido this is looking really good!

I've suggested two really minor changes, but this is ready for upstream review once they're addressed.

👍

  • I tested this by running the dev server:
    # Run migrations
    python3 manage.py migrate
    # Create a superuser
    python3 manage.py createsuperuser # and used the shell to create non-superusers
    # Run the dev server
    python3 manage.py runserver
    # Login using Django Admin (non-staff users can login, they just can't see Django Admin)
    # http://127.0.0.1:8000/admin/
    # REST API is here:
    # http://127.0.0.1:8000/tagging/rest_api/v1/
    
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation
  • Commit structure follows OEP-0051

@pomegranited pomegranited requested a review from ormsbee July 24, 2023 03:01
@rpenido rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from ff5dda3 to 4172286 Compare July 24, 2023 19:16
@rpenido rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from 4172286 to d532b0c Compare July 24, 2023 19:21
@pomegranited
Copy link
Contributor

Thanks for applying those changes @rpenido ! This is ready for @ormsbee and/or @bradenmacdonald 's review now.

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.

From a quick look, this looks fine. Just had two questions. I don't need to review this again; @pomegranited can give final approval and merge.

@rpenido
Copy link
Contributor Author

rpenido commented Jul 27, 2023

@pomegranited I made the changes with new commits: 2ccae75 and cec362d. That way, I think it is easier to review.

Let me know if I should squash the commits, or you can do it yourself.

Thank you!

@rpenido rpenido force-pushed the rpenido/fal-3449-taxonomy-view-management-apis branch from e2aeb9a to 3548151 Compare July 28, 2023 20:07
@pomegranited pomegranited merged commit 66c8742 into openedx:main Jul 31, 2023
@pomegranited pomegranited deleted the rpenido/fal-3449-taxonomy-view-management-apis branch July 31, 2023 00:01
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.

[Tagging] Taxonomy view/management APIs

4 participants