Single taxonomy view [FC-0030] #78

ChrisChV · 2023-09-08T23:04:58Z

Description

Adds a new view to return tags for a given taxonomy. With this view, you can:

Return paginated root tags.
Return paginated children of given tag.
Search tags in all taxonomy.
Search tags on a children list.

Also updates the ADR 0014 with some logics on search.

Supporting Information

Closes [Tagging] Single taxonomy view API modular-learning#75

Testing Instructions

Ensure that the tests cover the expected behavior of the view as described in the related issue.
Run this command to create a small taxonomy:

python manage.py loaddata tests/openedx_tagging/core/fixtures/tagging.yaml

Run this commands to create a large taxonomy:

python manage.py shell
> from tests.openedx_tagging.core.tagging.test_views import TestTaxonomyTagsView
> case = TestTaxonomyTagsView()
> case.setUp()
> case.build_large_taxonomy()
> from openedx_tagging.core.tagging.models import Taxonomy, Tag
> Taxonomy.objects.filter(name="Large Taxonomy")

Run django server: python manage.py runserver
You can comment this line to avoid the permissions.
Make a Get request to get the Languages tags. Verify that all tags are returned. Verify the structure.
Make a Get request to get the small taxonomy tags: http://127.0.0.1:8000/tagging/rest_api/v1/taxonomies/Tax-id/tags/
Verify that all tags are returned. Verify the tree structure.
Make a Get request to get the large taxonomy tags.
- Verify that the root tags are paginated.
- Verify the structure.
- Open the next page link.
- Verify the result.
- Open a children link.
- Verify the result.
Make a Get request to search tags on the small taxonomy: http://127.0.0.1:8000/tagging/rest_api/v1/taxonomies/Tax-id/tags/?search_term=eU
Verify the results.
Make a Get request to search tags on the large taxonomy: http://127.0.0.1:8000/tagging/rest_api/v1/taxonomies/Tax-id/tags/?search_term=10
Verify the results.

openedx-webhooks · 2023-09-08T23:05:02Z

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.

ChrisChV · 2023-09-13T21:01:19Z

@bradenmacdonald Do you have any idea why quality errors occur? We have the same syntax here and here

bradenmacdonald · 2023-09-13T23:55:13Z

@ChrisChV Yes, it's very simple - you need to put from __future__ import annotations at the top of the file :)
It's because Open edX is still using python 3.8.

bradenmacdonald · 2023-09-14T00:26:32Z

openedx_tagging/core/tagging/models/base.py

+            tag_set = self.tag_set.all()
+
+        if self.allow_free_text:
+            return tag_set.none()


I'm not really a big fan of the idea that we have to call different APIs to get the tags based on the type of taxonomy. It would be nice if the API could handle that implementation detail internally. We can think about this for later though.

Do you mean having two functions? get_tags and get_filtered_tags?

No I mean for example that it would be nice if search_tags worked for any taxonomy, but right now it only works for taxonomies that aren't free text. We can change that in the future though; no change for now. After all we have excluded free text from the MVP anyways.

openedx_tagging/core/tagging/models/base.py

openedx_tagging/core/tagging/rest_api/paginators.py

openedx_tagging/core/tagging/rest_api/v1/permissions.py

bradenmacdonald · 2023-09-18T20:21:46Z

openedx_tagging/core/tagging/rest_api/paginators.py

+
+class TagsPagination(DefaultPagination):
+    """
+    Custom paginator that returns the range index of the current page


Why is this useful/necessary? As far as I can see, we're not using it (yet).

It's always a good idea to put the "why" into the docstring so that in the future people know why it's here and can remove the code if it's not needed. If we don't say why and it never gets used, the code may sit around forever anyways, if nobody knows whether it's important or not.

If it's for openedx/modular-learning#111 , I'm actually going to suggest that we exclude that from the MVP or at least do it later as part of Step 4, not implement it now in Step 2.

It's always a good idea to put the "why" into the docstring so that in the future people know why it's here and can remove the code if it's not needed.

It is because of this discussion. The range index of the current page is shown in the UI

If it's for openedx/modular-learning#111 , I'm actually going to suggest that we exclude that from the MVP or at least do it later as part of Step 4, not implement it now in Step 2.

Ok, I see. In that case, should I remove everything related to range index?

I updated this here: 6dded73

Yeah, that's more of a UI thing. Especially when we're tight for budget, let's exclude it for now and we can consider adding it later when we implement that story. Also I think it makes more sense to implement it generally for the whole platform, not just for the tagging use case.

Also I think it makes more sense to implement it generally for the whole platform, not just for the tagging use case.

I have implemented it that way because of how special this tag view is. We have to change the paginator to disable pagination in that view.

Ah ok. Well if you want to keep what you have it's fine with me. I just don't want to spend any more time on it now.

openedx_tagging/core/tagging/rest_api/v1/views.py

tests/openedx_tagging/core/tagging/test_models.py

tests/openedx_tagging/core/tagging/test_views.py

bradenmacdonald · 2023-09-20T19:53:41Z

I'm not sure why the CLA check isn't working. Please rebase/squash this tomorrow and ping me to merge ; hopefully the check is fixed by then.

docs: ADR for pagination and repr of single taxonomy api feat: Initial view and pagination config feat: get_range_index added to paginator feat: View for get tags - Get paginated root tags - Get paginated children tags - Get all tags for small taxonomies - Tags structure for response chore: Adding permissions and docstrings style: quality feat: Search added to get tags view feat: searching tags docs: Adding comments on the view docs: Update ADR with search logic chore: fix quality checks chore: fix tests style: Nits on type hints and docstrings and comments fix: Delete all about range index in paginators

ChrisChV · 2023-09-20T20:17:19Z

I'm not sure why the CLA check isn't working. Please rebase/squash this tomorrow and ping me to merge ; hopefully the check is fixed by then.

@bradenmacdonald Done

openedx-webhooks · 2023-09-20T23:11:41Z

@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.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 8, 2023

ChrisChV marked this pull request as draft September 8, 2023 23:05

ChrisChV changed the title ~~Single taxonomy view~~ Single taxonomy view [FC-0030] Sep 8, 2023

ChrisChV marked this pull request as ready for review September 13, 2023 20:26

bradenmacdonald reviewed Sep 18, 2023

View reviewed changes

bradenmacdonald approved these changes Sep 20, 2023

View reviewed changes

ChrisChV force-pushed the chris/single-taxonomy-view branch from 6dded73 to d7e5872 Compare September 20, 2023 20:16

bradenmacdonald merged commit 69519f4 into openedx:main Sep 20, 2023
6 checks passed

bradenmacdonald deleted the chris/single-taxonomy-view branch September 20, 2023 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single taxonomy view [FC-0030] #78

Single taxonomy view [FC-0030] #78

ChrisChV commented Sep 8, 2023 •

edited

Loading

openedx-webhooks commented Sep 8, 2023 •

edited

Loading

ChrisChV commented Sep 13, 2023

bradenmacdonald commented Sep 13, 2023

bradenmacdonald Sep 14, 2023

ChrisChV Sep 20, 2023

bradenmacdonald Sep 20, 2023

bradenmacdonald Sep 18, 2023

bradenmacdonald Sep 20, 2023

ChrisChV Sep 20, 2023

ChrisChV Sep 20, 2023

ChrisChV Sep 20, 2023

bradenmacdonald Sep 20, 2023

ChrisChV Sep 20, 2023

bradenmacdonald Sep 20, 2023

bradenmacdonald commented Sep 20, 2023

ChrisChV commented Sep 20, 2023

openedx-webhooks commented Sep 20, 2023

Single taxonomy view [FC-0030] #78

Single taxonomy view [FC-0030] #78

Conversation

ChrisChV commented Sep 8, 2023 • edited Loading

Description

Supporting Information

Testing Instructions

openedx-webhooks commented Sep 8, 2023 • edited Loading

ChrisChV commented Sep 13, 2023

bradenmacdonald commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Sep 20, 2023

ChrisChV commented Sep 20, 2023

openedx-webhooks commented Sep 20, 2023

ChrisChV commented Sep 8, 2023 •

edited

Loading

openedx-webhooks commented Sep 8, 2023 •

edited

Loading