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

[8664] auto searchprofile names #5928

Merged
merged 1 commit into from
Jan 15, 2025
Merged

[8664] auto searchprofile names #5928

merged 1 commit into from
Jan 15, 2025

Conversation

goapunk
Copy link
Contributor

@goapunk goapunk commented Jan 9, 2025

Describe your changes
Add code to automatically name searchprofiles as Searchprofile 1, Searchprofile 2, etc.

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

@goapunk goapunk changed the base branch from main to dev January 9, 2025 16:05
@goapunk goapunk force-pushed the jd-2025-01-searchprofile-names branch from 068fb43 to f94581a Compare January 9, 2025 16:06
@goapunk goapunk force-pushed the jd-2025-01-searchprofile-names branch 3 times, most recently from acbcec1 to 525513c Compare January 13, 2025 16:19
@goapunk goapunk changed the title WIP: [8664] auto searchprofile names [8664] auto searchprofile names Jan 13, 2025
@goapunk goapunk force-pushed the jd-2025-01-searchprofile-names branch 2 times, most recently from 28f9ee4 to 9b13c2d Compare January 13, 2025 16:45
@goapunk goapunk requested a review from m4ra January 13, 2025 16:50
@goapunk goapunk force-pushed the jd-2025-01-searchprofile-names branch from 9b13c2d to 57acdbf Compare January 14, 2025 10:20
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

suggest to move the indexing in the save method.

@goapunk goapunk force-pushed the jd-2025-01-searchprofile-names branch from 57acdbf to 9d37783 Compare January 14, 2025 13:28
@goapunk goapunk requested a review from m4ra January 14, 2025 13:31
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

Left few comments, and I think we shouldn't let the name optional, but save it with the auto-generated function.

pass
self.number = latest.number + 1 if latest else 1
if update_fields:
update_fields.append("number")
Copy link
Contributor

Choose a reason for hiding this comment

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

update_fields is not a list, it needs a union IIRC. However, I would think number field shouldn't be part of updates, only saved upon creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I'll remove it

meinberlin/apps/kiezradar/models.py Show resolved Hide resolved
ordering = ["number"]
constraints = [
models.UniqueConstraint("user", "number", name="unique-search-profile")
]
Copy link
Contributor

Choose a reason for hiding this comment

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

we also need an index on the number field.

meinberlin/apps/kiezradar/serializers.py Show resolved Hide resolved
meinberlin/apps/kiezradar/models.py Show resolved Hide resolved
@vellip vellip mentioned this pull request Jan 15, 2025
5 tasks
@m4ra m4ra mentioned this pull request Jan 15, 2025
5 tasks
@m4ra
Copy link
Contributor

m4ra commented Jan 15, 2025

@goapunk looking at the story again, it asks that we always increase the index, even it existing profiles are deleted. But I cannot think of a simple way to do so, hence you may want to communicate this decision in the story for when the testing phase starts, they are aware.

@goapunk
Copy link
Contributor Author

goapunk commented Jan 15, 2025

@goapunk looking at the story again, it asks that we always increase the index, even it existing profiles are deleted. But I cannot think of a simple way to do so, hence you may want to communicate this decision in the story for when the testing phase starts, they are aware.

will do, its already in the comments of the story as well

@goapunk goapunk force-pushed the jd-2025-01-searchprofile-names branch from 9d37783 to 3edb32e Compare January 15, 2025 15:00
@goapunk goapunk requested a review from m4ra January 15, 2025 15:01
@sevfurneaux
Copy link
Collaborator

As Janek has confirmed we'll use this implementation for the naming, I'll wait for this PR to be merged and check all is working in my [#8661] Add 'Save Profile' button PR.

Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

thank you!

@m4ra m4ra merged commit 4710e3a into dev Jan 15, 2025
3 checks passed
@m4ra m4ra deleted the jd-2025-01-searchprofile-names branch January 15, 2025 17:18
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.

3 participants