Autocomplete tags API function by ChrisChV · Pull Request #59 · openedx/openedx-core

ChrisChV · 2023-06-20T20:10:21Z

Description

Implements autocomplete_tags function

Supporting information

Merge after #57.

Closes openedx/modular-learning#65

Testing instructions

Ensure that the tests cover the expected behaviour

openedx-webhooks · 2023-06-20T20:10:26Z

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-06-21T22:03:31Z

@pomegranited The CI tests are failing, but in my local works normally. Please let me know your results in your local

pomegranited · 2023-06-22T07:55:13Z

@pomegranited The CI tests are failing, but in my local works normally. Please let me know your results in your local

@ChrisChV Hmm.. make test passes for me locally, and I had the same problem on mine, so I think it's a sorting issue, something to do with CI running the tests on mysql?

If you merge my latest changes, it should fix it.

ChrisChV · 2023-06-22T19:56:56Z

@ChrisChV Hmm.. make test passes for me locally, and I had the same problem on mine, so I think it's a sorting issue, something to do with CI running the tests on mysql?

If you merge my latest changes, it should fix it.

@pomegranited Thanks. MySQL uses __istartswith to make case-insensitive searches.

ChrisChV · 2023-06-22T20:00:06Z

@ormsbee It's ready for your review

ChrisChV · 2023-06-27T22:39:05Z

@ormsbee I merged the changes of #57. Now the review is easier and it's ready

pomegranited

Couple of minor nits, but I've also got a question about duplicate Tags in a single taxonomy?

ormsbee

Mostly questions and requests around the database queries being made. Thank you!

ormsbee · 2023-06-29T15:21:37Z

+            .order_by('_value')
+            # obtain the values of the tags
+            .values_list('_value', flat=True)
+            # remove repeats


In what scenarios could we get repeats?

About the free-form taxonomies, we do the search in ObjectTag. There may be the scenario where different objects have the same tag

ormsbee · 2023-06-29T15:24:26Z

+            [:count]
+        )
+        # Build tag results
+        result = [TagResult(id=None, value=value) for value in result]


Given that this is likely to be used by a REST API at some point, I think it's fine to have a function that returns a QuerySet, even if this function just calls out to it and does this serialization into TagResult.

You are right. I have added a separate function for serialization, that can be used by other internal functions

ormsbee · 2023-06-29T15:30:28Z

+            # omit any tags applied to the given object
+            .exclude(object_id=object_id)
+            # omit any free-text tags from other objects whose values match the tags on the given object
+            .exclude(_value__in=excluded_tags)


If we're omitting everything with the object_id here, why do we need to check excluded_tags as well (which should just be a subset)?

It's the same logic of #59 (comment). There may be tags of other objects that the object_id already has. But I have realized that .exclude(object_id=object_id) is no necessary, because .exclude(_value__in=excluded_tags) also excludes the tags of object_id

mphilbrick211 · 2023-07-17T23:24:34Z

Hi @ChrisChV! Just following up on this :)

pomegranited · 2023-07-18T07:58:21Z

Hi @mphilbrick211 :) This is blocked on #62, but we're nearly there :)

mphilbrick211 · 2023-07-18T12:40:49Z

Hi @mphilbrick211 :) This is blocked on #62, but we're nearly there :)

Thanks, @pomegranited!

pomegranited

Hi @ChrisChV -- I've left some comments in light of the soon-to-be-approved #62

pomegranited · 2023-07-26T05:42:21Z


+    def autocomplete_tags(
+        self, prefix: str, object_id: str = None, count=10
+    ) -> models.QuerySet:


I think it's going to be terribly confusing that the QuerySet returned here is a ObjectTag queryset for free-text taxonomies, but a Tag queryset for closed taxonomies. So we can't simply use the API and treat them like the same thing.

Plus, for closed taxonomies, the frontend can handle auto-completion without consulting the backend (e.g using autocompleter), because it's already fetched all of the Tags in a Taxonomy to display the tree of options:

So I think we should:

Only autocomplete from existing ObjectTags, ignoring the free-text vs closed taxonomy distinction.

Match text anywhere in the value, not just prefix (as shown above), by using .filter(_value__icontains=text) instead

Remove the count parameter -- the REST API that ends up calling this can handle pagination from the returned query set.

Does that sound reasonable to you?

@pomegranited Yes, with this the function will be cleaner. The new function will only return the _value and tag. The second will be null for closed taxonomies

With this change, I wonder, won't there be other things that the frontend can simplify or change?

Updated here a8523b5

pomegranited

This is looking really solid, @ChrisChV .
I've suggested some improvements to the docstrings and an alias to make the returned tag values nicer, but will approve once they're applied.

bradenmacdonald

LGTM.

While I generally recommend against returning QuerySets in api.py methods*, I think this one makes sense because it's a ValuesQuerySet. I like how the method is returning values and tag, so that we can be more flexible in the future, e.g. returning users even if their Tags don't exist yet.

I have a few questions/suggestions, but nothing blocking.

Nice work.

* Because: "An app’s Django models and other internal data structures should not be exposed via its Python APIs"

pomegranited · 2023-08-02T01:49:36Z

@bradenmacdonald

While I generally recommend against returning QuerySets in api.py methods because: "An app’s Django models and other internal data structures should not be exposed via its Python APIs"

Huh ok.. I didn't know that, and we definitely should have picked this up in the design phase.

My assumption was that the REST APIs should be as think as possible, and so should use the Python APIs (and django model rules) to encapsulate any complexity. And since we use DRF, that means preferring the Python APIs to return QuerySets.

How should this be done instead?

pomegranited · 2023-08-02T01:50:49Z

@ChrisChV could you rebase this on latest main and bump the version so we can merge?

To return a list of TagResult containing the id and value of the tags.

We need to use __istartswith to make a case-insensitive query on MySQL

- Match text anywhere in the value - Only autocomplete from existing ObjectTags, ignoring the free-text vs closed taxonomy distinction. - Remove the count parameter - Refactoring tests to use ddt

bradenmacdonald · 2023-08-02T16:17:34Z

@pomegranited It's not a hard and fast rule. The main problem the authors of that OEP are trying to avoid is that Django model instances are very "powerful" and once you have access to one, you can make a lot of "internal" changes that may violate the assumptions of the API - for example, you can change fields and save() it, bypassing whatever save_changes() API may exist. Specific example: In the tagging case, we'd generally want people to use the tag_object() API method, not modifying or deleting an ObjectTag instance themselves. In fact, that sort of design is one way to avoid having so many subclasses - by forcing people to use the methods like tagging_api.tag_object(...) you can keep all the extra validation/creation logic in that method, and avoid the need for model subclasses.

So: How can you make the REST APIs a thin wrapper around the python API, without using Django models? You can look at this example in content_libraries v2 as one example I'm familiar with. Other times it may make more sense to use a query set in the REST API if it makes your life a lot easier (e.g. pagination). Yet another option is have an "internal API" that uses QuerySets and then your REST API can wrap it directly and your Python API can wrap it and convert the Django objects to non-Django data structures. As far as I know, any of those are totally fine, so just go with any approach that feels clean to you. Just keep in mind that exposing Django model instances via python will tempt people to bypass your python API, which you probably want to discourage.

ChrisChV · 2023-08-02T16:42:04Z

@ChrisChV could you rebase this on latest main and bump the version so we can merge?

@pomegranited Done 👍

pomegranited

👍 Thank you @ChrisChV !

I tested this by checking that the tests correctly cover the expected behavioral use cases.
I read through the code.
~~I checked for accessibility issues~~ N/A
Includes documentation -- docstrings

pomegranited · 2023-08-03T01:51:05Z

Hi @ormsbee -- have your concerns been addressed? Github won't let me merge until you say it's ok :)

I don't want to "dismiss" this review -- Dave's questions were very relevant and appreciated. But they've been answered, and we had a second approval from Braden, so I think it's safe to merge this.

openedx-webhooks · 2023-08-06T23:13:53Z

@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 Jun 20, 2023

ChrisChV marked this pull request as draft June 20, 2023 20:10

ChrisChV changed the title ~~Autocomplete tags API function (WIP)~~ Autocomplete tags API function Jun 20, 2023

ChrisChV force-pushed the chris/autocomplete-tags branch from 0b9e6f6 to e8f8244 Compare June 21, 2023 00:26

ChrisChV marked this pull request as ready for review June 21, 2023 00:27

pomegranited requested changes Jun 21, 2023

View reviewed changes

ChrisChV force-pushed the chris/autocomplete-tags branch 2 times, most recently from 0642778 to 4923b66 Compare June 21, 2023 21:56

pomegranited reviewed Jun 22, 2023

View reviewed changes

Comment thread openedx_tagging/core/tagging/models.py Outdated

ChrisChV force-pushed the chris/autocomplete-tags branch from 4923b66 to 67bb621 Compare June 22, 2023 19:07

ChrisChV force-pushed the chris/autocomplete-tags branch from 89e6455 to 2815e58 Compare June 27, 2023 22:38

pomegranited requested changes Jun 28, 2023

View reviewed changes

Comment thread openedx_tagging/core/tagging/api.py Outdated

Comment thread openedx_tagging/core/tagging/models.py Outdated

Comment thread openedx_tagging/core/tagging/api.py Outdated

ormsbee previously requested changes Jun 29, 2023

View reviewed changes

pomegranited requested changes Jul 19, 2023

View reviewed changes

Comment thread tests/openedx_tagging/core/tagging/test_api.py Outdated

Comment thread openedx_tagging/core/tagging/models.py

Comment thread openedx_tagging/core/tagging/api.py Outdated

Comment thread openedx_tagging/core/tagging/api.py Outdated

ChrisChV force-pushed the chris/autocomplete-tags branch from adb3e5a to 507848b Compare July 25, 2023 18:06

pomegranited requested changes Jul 26, 2023

View reviewed changes

pomegranited requested changes Jul 27, 2023

View reviewed changes

bradenmacdonald reviewed Jul 27, 2023

View reviewed changes

Comment thread openedx_tagging/core/tagging/models.py

bradenmacdonald approved these changes Jul 27, 2023

View reviewed changes

Comment thread openedx_tagging/core/tagging/api.py

Comment thread openedx_tagging/core/tagging/api.py

ChrisChV added 13 commits August 2, 2023 10:56

feat: Autocomplete tags

bbb24e6

chore: Adding comments, fixing some cases

8ce5f8e

chore: autocomplete_tags function updated

f67bacb

To return a list of TagResult containing the id and value of the tags.

chore: Fix MySQL case-sensitive error

2b7948b

We need to use __istartswith to make a case-insensitive query on MySQL

chore: Duplicate tags behavior updated

7316786

chore: Index and query updates

ccd82de

chore: autocomplete_tags_result added

72005a7

chore: Unique constraint on Tag model added

56a2e3e

feat: unique constraint to ObjectTag

c5e4bb5

feat: function moved to Taxonomy model

3ac4cbc

refactor: autocomplete function

61d9a7d

- Match text anywhere in the value - Only autocomplete from existing ObjectTags, ignoring the free-text vs closed taxonomy distinction. - Remove the count parameter - Refactoring tests to use ddt

style: nits, lint and docs

71a1428

style: Nits

f0fef57

chore: Rebase with main and bump version

ea01201

ChrisChV force-pushed the chris/autocomplete-tags branch from 7c30263 to ea01201 Compare August 2, 2023 16:35

pomegranited approved these changes Aug 3, 2023

View reviewed changes

pomegranited mentioned this pull request Aug 3, 2023

[Tagging] Clean up Tagging models openedx/modular-learning#85

Closed

pomegranited merged commit fb1fd08 into openedx:main Aug 6, 2023

pomegranited deleted the chris/autocomplete-tags branch August 6, 2023 23:13

ChrisChV mentioned this pull request Aug 15, 2023

docs: ADR for pagination and repr of single taxonomy view api [FC-0030] #72

Merged

ormsbee mentioned this pull request Mar 24, 2025

add vscode pytest settings open-craft/openedx-learning#19

Closed

ormsbee mentioned this pull request Sep 4, 2025

version bump 0.28.0 open-craft/openedx-learning#20

Closed

Uh oh!

Conversation

ChrisChV commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Uh oh!

openedx-webhooks commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChrisChV commented Jun 21, 2023

Uh oh!

pomegranited commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ChrisChV commented Jun 22, 2023

Uh oh!

ChrisChV commented Jun 22, 2023

Uh oh!

ChrisChV commented Jun 27, 2023

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphilbrick211 commented Jul 17, 2023

Uh oh!

pomegranited commented Jul 18, 2023

Uh oh!

mphilbrick211 commented Jul 18, 2023

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisChV Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChrisChV commented Jun 20, 2023 •

edited

Loading

openedx-webhooks commented Jun 20, 2023 •

edited

Loading

pomegranited commented Jun 22, 2023 •

edited

Loading

ChrisChV Jul 26, 2023 •

edited

Loading

bradenmacdonald left a comment •

edited

Loading