[#8661] Add 'Save Profile' button #5940

sevfurneaux · 2025-01-15T11:59:16Z

Describe your changes
This PR adds the "Save Search Profile" button.

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

sevfurneaux · 2025-01-15T12:08:50Z

@vellip Thanks for reviewing the other PR before it inadvertently closed! I've tackled all of code changes and commit suggestions from the previous PR in this one.

I'll list what I commented on, so we can keep track of them (or talk about them in here):

At this point it might've made more sense to use a react context. But since that would now be a larger rework, it's fine as is.

I thought the name is generated by the backend?

Why does it need that?

I think we usually don't display a "Login to do..." link, but we show what it normally looks like and if not authenticated then redirect to the login page.

Should we add an anchor link to the particular profile in question? ...#search-profile-3

sevfurneaux · 2025-01-15T12:16:32Z

@goapunk @m4ra @vellip in regards to the name generation, this is currently happening in the frontend and is implemented in this PR. If a user selects any of the filters, then based on what filters/search query are available the names are created:

Example where districts filters are used

Example where search query is used

m4ra · 2025-01-15T13:35:10Z

@sevfurneaux the name is generated as SearchProfile 1 SearchProfile 2, etc and it's being developed in this PR #5928
It's best to let us know in our channels if you work on something that is not in the scope of the story.

sevfurneaux · 2025-01-15T13:39:33Z

Thanks @m4ra. I actually implemented this due to it being noted in the #8661
[mB] Kiezradar frontend: "save search" button on Project Overview page ticket:

If a search profile does not have a name, automatically generate a name based on the metadata of filters picked in the project overview page.

But presumably it wasn't updated prior to the ticket being created for the backend implementation.

Hopefully that won't happen again! 🤞

Ok, so shall I remove my implementation? Presumably you'll add the additional functionality beyond SearchProfile 1 and SearchProfile 2 so that it includes the metdata such as Projects in Charlottenburg as per the design? Unsure if that is noted in your ticket.

m4ra · 2025-01-15T14:04:03Z

@sevfurneaux That's getting more confusing now. No, we are not implementing naming based on metadata. @janeklqd can you please clarify whether we save the names as autogenerated with searchprofile 1.... etc or from the metadata. Adding here @goapunk since he has a PR open for the auto-naming in the backend.

Thanks @m4ra. I actually implemented this due to it being noted in the #8661
[mB] Kiezradar frontend: "save search" button on Project Overview page ticket:
If a search profile does not have a name, automatically generate a name based on the metadata of filters picked in the project overview page.
But presumably it wasn't updated prior to the ticket being created for the backend implementation.

Hopefully that won't happen again! 🤞

Ok, so shall I remove my implementation? Presumably you'll add the additional functionality beyond SearchProfile 1 and SearchProfile 2 so that it includes the metdata such as Projects in Charlottenburg as per the design? Unsure if that is noted in your ticket.

m4ra · 2025-01-15T14:18:40Z

meinberlin/apps/plans/views.py

-        districts = AdministrativeDistrict.objects.all()
-        district_names_list = [district.name for district in districts]
-        district_names_list.append(str(city_wide))
-        return json.dumps(district_names_list)


@goapunk any idea if the district polygons are used anywhere outside the plans, anything to do with bplans? Since @sevfurneaux is removing these methods, would be good to double check with you as well.

@sevfurneaux the map on the project list will not be highlighting the district borders any more? If that's the case I think removing this is fine

m4ra

Looks good, only a check for the user that is not required, and waiting for Julien to confirm we are not in need of the district_polygons context.

m4ra · 2025-01-15T14:20:37Z

meinberlin/apps/plans/views.py

@@ -119,6 +137,12 @@ def get_search_profile(self):
                pass
        return None

+    def get_search_profiles_count(self):
+        if self.request.user.is_authenticated:


AFAIK no need for this check, as the view is allowed only to registered users. Same in the get_search_profile method above

Yup, verified, we don't need these authentication checks, they are inherited from adhocracy4 as a mixin in this view class declaration above.

Great, thanks. But this view is for the plans map (/projects) page, so users can be unauthenticated here, so we'll still need it?

agreed, as this page can be viewed by anyone I think we need the check

sevfurneaux · 2025-01-15T15:58:44Z

Looks good, only a check for the user that is not required, and waiting for Julien to confirm we are not in need of the district_polygons context.

@m4ra thank you! Looks like there's coverage/coveralls errors, presumably I need to add tests for the views.py changes?

sevfurneaux · 2025-01-15T16:34:08Z

@goapunk @m4ra Janek has confirmed we'll go with the backend auto-generated naming in #5928, so I'll remove my frontend implementation 👍

vellip · 2025-01-15T17:04:23Z

meinberlin/react/projects/ProjectsControlBar.jsx

+
+  useEffect(() => {
+    const handlePopState = () => {
+      window.location.reload()


Did you try replaceState instead of pushState so we don't need this?

Just changed to replaceState 👍. I misunderstood replaceState, thanks! So now when the user uses back/forward it goes to the previous page.

m4ra · 2025-01-15T17:39:31Z

We should usually, but let's move on with this. Once Julian confirms, you can merge it.

sevfurneaux · 2025-01-16T09:14:45Z

Hi 👋 @goapunk, I think @m4ra is away now. She had a question above about district_polygons above. Then there is the coveralls error about uncovered code: do we need to update a test to get this green? Or is there some kind of approval in coveralls?

goapunk · 2025-01-16T12:27:54Z

Hi 👋 @goapunk, I think @m4ra is away now. She had a question above about district_polygons above. Then there is the coveralls error about uncovered code: do we need to update a test to get this green? Or is there some kind of approval in coveralls?

@sevfurneaux hey :) will have a look later today!

goapunk

@sevfurneaux looks good to me, I left a question for you regarding the district polygons, if the answer is positive then go ahead and merge. Ignoring the coveralls check is ok

goapunk · 2025-01-20T12:38:32Z

meinberlin/apps/plans/views.py

-        districts = AdministrativeDistrict.objects.all()
-        district_names_list = [district.name for district in districts]
-        district_names_list.append(str(city_wide))
-        return json.dumps(district_names_list)


@sevfurneaux the map on the project list will not be highlighting the district borders any more? If that's the case I think removing this is fine

goapunk · 2025-01-20T12:39:30Z

meinberlin/apps/plans/views.py

@@ -119,6 +137,12 @@ def get_search_profile(self):
                pass
        return None

+    def get_search_profiles_count(self):
+        if self.request.user.is_authenticated:


agreed, as this page can be viewed by anyone I think we need the check

sevfurneaux · 2025-01-20T12:57:11Z

Thanks for reviewing @goapunk.

@sevfurneaux the map on the project list will not be highlighting the district borders any more? If that's the case I think removing this is fine

From testing, get_district_polygons was used in context["districts"], but the context["districts"] data was never directly rendered or manipulated in any of the React components. For example on dev, districts is passed into the ProjectsControlBar here: https://github.com/liqd/a4-meinberlin/blob/dev/meinberlin/react/projects/ProjectsListMapBox.jsx#L117

...but ProjectsControlBar does not reference it from its component props: https://github.com/liqd/a4-meinberlin/blob/dev/meinberlin/react/projects/ProjectsControlBar.jsx#L56-L65

vellip · 2025-01-20T13:04:21Z

Thanks for reviewing @goapunk.

@sevfurneaux the map on the project list will not be highlighting the district borders any more? If that's the case I think removing this is fine

I never saw any designs highlighting district borders anymore with the redesign.

sevfurneaux self-assigned this Jan 15, 2025

sevfurneaux requested a review from vellip January 15, 2025 12:08

sevfurneaux mentioned this pull request Jan 15, 2025

[#8661] Add 'Save Profile' button #5930

Closed

5 tasks

sevfurneaux force-pushed the sf-2025-add-save-profile-search-action branch 2 times, most recently from a3fa4d1 to 1a072c4 Compare January 15, 2025 13:33

sevfurneaux requested a review from m4ra January 15, 2025 13:49

m4ra reviewed Jan 15, 2025

View reviewed changes

sevfurneaux mentioned this pull request Jan 15, 2025

[8664] auto searchprofile names #5928

Merged

5 tasks

vellip reviewed Jan 15, 2025

View reviewed changes

sevfurneaux force-pushed the sf-2025-add-save-profile-search-action branch from 1a072c4 to b3826a4 Compare January 15, 2025 18:54

sevfurneaux requested review from vellip, m4ra and goapunk January 15, 2025 18:57

goapunk approved these changes Jan 20, 2025

View reviewed changes

search-profile: add search profile button

f6645e4

sevfurneaux force-pushed the sf-2025-add-save-profile-search-action branch from b3826a4 to f6645e4 Compare January 20, 2025 13:08

sevfurneaux merged commit 218ffc3 into dev Jan 20, 2025
2 of 3 checks passed

sevfurneaux deleted the sf-2025-add-save-profile-search-action branch January 20, 2025 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#8661] Add 'Save Profile' button #5940

[#8661] Add 'Save Profile' button #5940

sevfurneaux commented Jan 15, 2025

sevfurneaux commented Jan 15, 2025

sevfurneaux commented Jan 15, 2025

m4ra commented Jan 15, 2025

sevfurneaux commented Jan 15, 2025 •

edited

Loading

m4ra commented Jan 15, 2025 •

edited

Loading

m4ra Jan 15, 2025

goapunk Jan 20, 2025

m4ra left a comment

m4ra Jan 15, 2025

m4ra Jan 15, 2025

sevfurneaux Jan 15, 2025 •

edited

Loading

goapunk Jan 20, 2025

sevfurneaux commented Jan 15, 2025 •

edited

Loading

sevfurneaux commented Jan 15, 2025

vellip Jan 15, 2025

sevfurneaux Jan 15, 2025

m4ra commented Jan 15, 2025

sevfurneaux commented Jan 16, 2025

goapunk commented Jan 16, 2025

goapunk left a comment

goapunk Jan 20, 2025

goapunk Jan 20, 2025

sevfurneaux commented Jan 20, 2025 •

edited

Loading

vellip commented Jan 20, 2025

[#8661] Add 'Save Profile' button #5940

[#8661] Add 'Save Profile' button #5940

Conversation

sevfurneaux commented Jan 15, 2025

sevfurneaux commented Jan 15, 2025

sevfurneaux commented Jan 15, 2025

m4ra commented Jan 15, 2025

sevfurneaux commented Jan 15, 2025 • edited Loading

m4ra commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4ra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sevfurneaux Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sevfurneaux commented Jan 15, 2025 • edited Loading

sevfurneaux commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4ra commented Jan 15, 2025

sevfurneaux commented Jan 16, 2025

goapunk commented Jan 16, 2025

goapunk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sevfurneaux commented Jan 20, 2025 • edited Loading

vellip commented Jan 20, 2025

sevfurneaux commented Jan 15, 2025 •

edited

Loading

m4ra commented Jan 15, 2025 •

edited

Loading

sevfurneaux Jan 15, 2025 •

edited

Loading

sevfurneaux commented Jan 15, 2025 •

edited

Loading

sevfurneaux commented Jan 20, 2025 •

edited

Loading