Filter by org badge #3155

magopian · 2024-09-25T13:21:16Z

Filter by org badge in

…` decorator

maudetes · 2024-09-27T12:55:37Z

As a suggestion, I would recommend to add it on datasets first in this PR as a first iteration and the others in another go.
It would probably allow for unit increments instead of maybe a big PR?

magopian · 2024-09-30T07:49:11Z

Yes, good idea, and at the same time... I think i'd like to keep all this in the same PR.
So, the reasoning is this: we currently have objects using the old system, and some using the new system.

datasets: old
reuses: new
dataservices: new
organizations: old

And those won't use exactly the same code, as their API, and more specifically, their parser aren't the same between the old and the new system.

Implementing at least one for each (dataset is already done in this PR, and reuse is nearly done), allows us to see what can be common between the two (maybe nothing).

And once we have one for each, implementing for the other two (organization and dataservice) should be very straightforward.

The other option would be to implement for the old system (eg datasets) in a different PR than for the new system (eg reuses), but modifications (following a review) might have implications in the other.
And then it would mean that I'd have to wait for the reuses implementation to be finalized before starting the dataservices implementation to avoid the extra work needed to maintain a PR based on another unmerged PR (eg the dataservices PR based on the reuses PR).

What do you think?

magopian · 2024-09-30T15:54:57Z

I believe this is now ready for review, with the one (big) caveat that this doesn't implement anything for the udata search service side of the feature, which will be done in a separate PR.

I'm not sure if this PR can be landed (after review and fixes obviously) before the change are done in udata-search-service? Haven't checked yet if passing an organization_badge filter which isn't supported/implemented yet will just ignore it, or return a failure?

bolinocroustibat

I'm not sure I can grasp the full features of the PR, but LGTM! Great job!

bolinocroustibat · 2024-10-01T13:23:31Z

udata/tests/api/test_dataservices_api.py

 from udata.core.dataservices.factories import DataserviceFactory
 from udata.core.dataservices.models import Dataservice
 from udata.core.dataset.factories import DatasetFactory, LicenseFactory
 from udata.core.organization.factories import OrganizationFactory
 from udata.core.organization.models import Member
 from udata.core.user.factories import UserFactory
 from udata.i18n import gettext as _
-from udata.tests.helpers import assert200, assert_redirects
+from udata.tests.helpers import assert200, assert400, assert_redirects


Very slightly bothered by the non-pythonic naming of those functions with no underscore, but that's just me 😆

yeah, it's a mix between pythonic and "unittest-ic" 😬

maudetes

I think it works smoothly, well done! 👏
I have only minor questions/remarks!

Notes:

The existing duplicate logic for the search makes me sad and makes your work more tedious
What happened with the badge organization constants being a mix of capitalize case and kebab case?

udata/core/dataset/api.py

maudetes · 2024-10-02T06:56:10Z

udata/core/organization/models.py

@@ -82,6 +82,9 @@ def hidden(self):
    def get_by_id_or_slug(self, id_or_slug):
        return self(slug=id_or_slug).first() or self(id=id_or_slug).first()

+    def with_badge(self, kind):


Since the QuerySet is quite straightforward, do we need to create this helper?

sooooo we don't really need it, but it's very convenient for the generate_fields.related_fields declaration here.

I can find a way around it if you feel we should, let me know.

Yes, I think I then saw the usage is related_fields and thought "yes ok, seems fair"! Let's keep it!

udata/api_fields.py

udata/core/dataset/search.py

maudetes · 2024-10-02T12:31:36Z

udata/core/reuse/models.py

        {"key": "datasets", "value": "metrics.datasets"},
        {"key": "followers", "value": "metrics.followers"},
        {"key": "views", "value": "metrics.views"},
    ],
+    related_filters=[


I would be curious about @ThibaudDauce opinion on this part :)

Hum, I would try to make it work with only:

additional_filters=[ 'organization.badge', ]

but not sure it's possible :-) If you want to pair program on this @magopian we could look into it :-)

udata/core/reuse/api.py

udata/api_fields.py

magopian · 2024-10-02T14:09:22Z

The existing duplicate logic for the search makes me sad and makes your work more tedious

Agreed. Should we tackle that in a future PR?

What happened with the badge organization constants being a mix of capitalize case and kebab case?

ah, I'm REALLY curious about that too, was pondering if it made sense to change it to something coherent. Should I give it a go in this PR? Or maybe in a future refactor/cleanup?

maudetes · 2024-10-03T07:19:33Z

The existing duplicate logic for the search makes me sad and makes your work more tedious

Agreed. Should we tackle that in a future PR?

Yes! Ticket created : datagouv/data.gouv.fr#1514

What happened with the badge organization constants being a mix of capitalize case and kebab case?

ah, I'm REALLY curious about that too, was pondering if it made sense to change it to something coherent. Should I give it a go in this PR? Or maybe in a future refactor/cleanup?

Let's do this in an upcoming PR, ticket created: datagouv/data.gouv.fr#1515
But I think we should do that next to implement the front with the correct filter wording directly.

And use inheritance instead of a `get_badge_mixin` helper in the inheritance declaration of the model.

magopian · 2024-10-17T10:44:31Z

@maudetes is that what you were thinking about?

maudetes

Thank you for all the work on this org badge! 💪
I find the result quite clean, well done!

I've created a PR on the search-service side: opendatateam/udata-search-service#49

udata/core/reuse/models.py

maudetes

👌

@ThibaudDauce

This is based on PR #3155 Add docstrings, comments and tests for the "new system" from @ThibaudDauce. --------- Co-authored-by: Thibaud Dauce <[email protected]>

magopian added 6 commits September 25, 2024 15:07

Add a .with_badge helper to the OrganizationQuerySet

6b7b912

Filter datasets by org badge in api

cc0d0cb

Add test for the dataset list api filtered on org badge

9cde4df

Filter dataset search

4eae2a9

Show a select in the swagger API for fields wich choices

216d7f8

Add a new "additional_filterables" parameter in the `@generate_fields…

bc8524b

…` decorator

magopian added 5 commits September 30, 2024 16:15

Fix datasets "filter on org badge" test

75b70cd

Filter reuses by org badge in api and search

d2faccf

Filter dataservices by org badge in api

7c11058

Filter organizations by badge in the api and search

b2a9d4d

Fix org badges serialization in search results

c231fa0

magopian marked this pull request as ready for review September 30, 2024 15:51

magopian requested review from maudetes, bolinocroustibat and ThibaudDauce September 30, 2024 15:54

bolinocroustibat approved these changes Oct 1, 2024

View reviewed changes

maudetes approved these changes Oct 2, 2024

View reviewed changes

@maudetes' review

52b7d2b

magopian and others added 2 commits October 9, 2024 16:08

Add choices to Badge.kind

fde3cc8

Filter by org badge complex

b4c2e2d

magopian force-pushed the filter-by-org-badge branch from 1ebc4eb to c87b5e2 Compare October 9, 2024 16:28

Fix tests

4cef617

magopian force-pushed the filter-by-org-badge branch from c87b5e2 to 4cef617 Compare October 9, 2024 16:39

Define a specific badge for each model using a badge mixin

362acf2

And use inheritance instead of a `get_badge_mixin` helper in the inheritance declaration of the model.

magopian mentioned this pull request Oct 22, 2024

Libify api fields #3174

Merged

magopian force-pushed the filter-by-org-badge branch from c2f0d58 to 362acf2 Compare October 22, 2024 16:35

Merge branch 'master' into filter-by-org-badge

d99f804

maudetes mentioned this pull request Oct 30, 2024

Add badge organization filter on dataset & reuse opendatateam/udata-search-service#49

Merged

maudetes approved these changes Oct 30, 2024

View reviewed changes

udata/core/reuse/models.py Outdated Show resolved Hide resolved

Use a custom parser arg name for the additional filters

9557800

maudetes approved these changes Nov 4, 2024

View reviewed changes

Merge branch 'master' into filter-by-org-badge

5b5bbc7

magopian merged commit 1f966a6 into master Nov 4, 2024
1 check passed

maudetes mentioned this pull request Nov 7, 2024

Soucis de validation de badge des datasets datagouv/data.gouv.fr#1564

Closed

magopian added a commit that referenced this pull request Nov 12, 2024

Libify api fields (#3174)

5921e14

This is based on PR #3155 Add docstrings, comments and tests for the "new system" from @ThibaudDauce. --------- Co-authored-by: Thibaud Dauce <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter by org badge #3155

Filter by org badge #3155

magopian commented Sep 25, 2024 •

edited

Loading

maudetes commented Sep 27, 2024

magopian commented Sep 30, 2024

magopian commented Sep 30, 2024

bolinocroustibat left a comment

bolinocroustibat Oct 1, 2024

magopian Oct 1, 2024

maudetes left a comment •

edited

Loading

maudetes Oct 2, 2024

magopian Oct 2, 2024

maudetes Oct 3, 2024

maudetes Oct 2, 2024

ThibaudDauce Oct 3, 2024

magopian commented Oct 2, 2024

maudetes commented Oct 3, 2024

magopian commented Oct 17, 2024

maudetes left a comment

maudetes left a comment

Filter by org badge #3155

Filter by org badge #3155

Conversation

magopian commented Sep 25, 2024 • edited Loading

maudetes commented Sep 27, 2024

magopian commented Sep 30, 2024

magopian commented Sep 30, 2024

bolinocroustibat left a comment

Choose a reason for hiding this comment

bolinocroustibat Oct 1, 2024

Choose a reason for hiding this comment

magopian Oct 1, 2024

Choose a reason for hiding this comment

maudetes left a comment • edited Loading

Choose a reason for hiding this comment

maudetes Oct 2, 2024

Choose a reason for hiding this comment

magopian Oct 2, 2024

Choose a reason for hiding this comment

maudetes Oct 3, 2024

Choose a reason for hiding this comment

maudetes Oct 2, 2024

Choose a reason for hiding this comment

ThibaudDauce Oct 3, 2024

Choose a reason for hiding this comment

magopian commented Oct 2, 2024

maudetes commented Oct 3, 2024

magopian commented Oct 17, 2024

maudetes left a comment

Choose a reason for hiding this comment

maudetes left a comment

Choose a reason for hiding this comment

magopian commented Sep 25, 2024 •

edited

Loading

maudetes left a comment •

edited

Loading