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

[8473] add kiezradar models #5740

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

m4ra
Copy link
Contributor

@m4ra m4ra commented Nov 12, 2024

Describe your changes

  • A Kiezradar query with a text field which can accommodate free text query.
  • A search profile related to user, corresponds to the project overview filters, and which has a m2m relation with the existing project overview filters. The idea is that a filter can be attached to different Search Profiles.
  • A Serializer with a custom update() to clear existing m2m relations and update them on editing existing SearchProfile.

Issues:
How to avoid user creating Search Profiles that are very similar. Maybe also add a limitation on how many search profiles they can have, as it can quickly scale up.

Tasks

  • PR name contains story or task reference
  • Steps to recreate and test the changes
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@m4ra m4ra changed the title apps: add kiezradar models [8473] add kiezradar models Nov 12, 2024
@m4ra m4ra changed the title [8473] add kiezradar models WIP [8473] add kiezradar models Nov 12, 2024
@m4ra m4ra force-pushed the mk-2024-11-kiezradar-search-profile-dev branch 2 times, most recently from 48ccbd5 to 312553f Compare November 12, 2024 15:59
@m4ra m4ra changed the title WIP [8473] add kiezradar models [8473] add kiezradar models Nov 12, 2024
@m4ra m4ra requested a review from goapunk November 12, 2024 16:06
search_profile = serializer.save()

# Define topics or retrieve from request data
topics = self.request.data.get("topics", ["Default Topic 1", "Default Topic 2"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the default ones for testing purposes.

Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

Added some questions and a note regarding providing_args

meinberlin/apps/kiezradar/signal.py Outdated Show resolved Hide resolved


class KiezRadarFilter(models.Model):
topic = models.CharField(max_length=126, unique=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure if I follow the structure here, do we need the KiezRadarFilter because it will be extended at some point in the future? Because if it only has the topic we could also just have a m2m to the Topic model on the SearchProfile, right? And in the case here: would it make more sense for the topic to be a foreign key on Topic instead of a CharField ? This way only topics which are defined can be added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood the multiselect filter later will have options that are added either by admins/initiators, or on the fly like keywords, while creating the search profile, not from the topics which come from the django settings. Also the topic field here could be a json field which combines different topics. Need to ask PM for clarification.

Copy link
Contributor

@goapunk goapunk Nov 18, 2024

Choose a reason for hiding this comment

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

my understanding of the story was that currently we only add the existing filters:

  • project title (free text)
  • organisation (m2m)
  • type (informational, participation, ...) (multi select)
  • status (all, running, ended) (single select)
  • district (m2m)
  • topic (as in the settings) (m2m)

and you can select multiple options per filter (except for status filter). Maybe we can talk about it in daily again with the responsible PM as I don't remember anything about admins/initiators being able to add options so might be good to clarify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified now, topics are the one predefined. Bur multiple filters for all categories, will result in search profiles that are almost similar for different free text searches, and too many relations.

topics = self.request.data.get("topics", ["Default Topic 1", "Default Topic 2"])

# Send the custom signal with topics
add_kiezradars_to_profile.send(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a signal here? can't we just add the topics to the search_profile directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the m2m relation, search profile needs to be saved first in order to add topics, that's why the signal is post_save.

@m4ra m4ra force-pushed the mk-2024-11-kiezradar-search-profile-dev branch from 312553f to 33f6706 Compare November 19, 2024 18:07
@m4ra m4ra requested a review from goapunk November 19, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants