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

style(models/User.public_id): add help_text #4397

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

calummackervoy
Copy link
Contributor

🤔 Pourquoi ?

Accès au Utilisateur par public_id plus efficace

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

@calummackervoy calummackervoy added python Pull requests that update Python code performance Amélioration des perfs labels Jul 8, 2024
@calummackervoy calummackervoy self-assigned this Jul 8, 2024
@calummackervoy calummackervoy force-pushed the calum/db-index-user-public-id branch from 8999756 to af9f1d6 Compare July 8, 2024 15:17
Copy link
Contributor

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Je l'aurais imaginé au sein de l'autre PR (car c'est à cause de l'autre PR que cet index est nécessaire), mais ça peut être une PR préparatoire.
Idéalement il faudrait dire dans le message de commit que c'est pour préparer le prochain ticket où l'on va beaucoup requêter les job seeker par public_id.

model_name="user",
name="public_id",
field=models.UUIDField(
db_index=True, default=uuid.uuid4, unique=True, verbose_name="identifiant public opaque, pour les API"
Copy link
Contributor

Choose a reason for hiding this comment

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

Tu pourrais au passage en profiter pour mettre à jour le verbose_name "pour les API et les URLs publiques"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai fais :

verbose_name="identifiant public",
help_text="identifiant opaque, pour les API et les URLs publiques",

Ça te va ? :)

@calummackervoy calummackervoy added no-changelog Ne doit pas figurer dans le journal des changements. modifié and removed no-changelog Ne doit pas figurer dans le journal des changements. labels Jul 9, 2024
Copy link
Contributor

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

En fait je me rend compte un peu tardivement que cela ne sert pas car l'index existe déjà via la contrainte d'unicité.
On peut néanmoins garder le changement de verbose_name en l'intégrant dans la migration 0008.

fix: code style

feat(User): added help_text to public_id field

fix(User): remove db_index which is implicit
@calummackervoy calummackervoy force-pushed the calum/db-index-user-public-id branch from b19010e to 2ee28bb Compare July 9, 2024 12:19
@calummackervoy calummackervoy added no-changelog Ne doit pas figurer dans le journal des changements. and removed performance Amélioration des perfs modifié labels Jul 9, 2024
@calummackervoy calummackervoy changed the title Performances: add db_index on User.public_id style(models/User.public_id): add help_text Jul 9, 2024
@calummackervoy
Copy link
Contributor Author

calummackervoy commented Jul 9, 2024

Ah oui t'as raison (https://docs.djangoproject.com/en/5.0/ref/models/fields/#unique)

OK, merci - j'ai retiré l'addition de db_index en gardant les autres changements

@calummackervoy calummackervoy added this pull request to the merge queue Jul 9, 2024
Merged via the queue into master with commit cd1246d Jul 9, 2024
18 checks passed
@calummackervoy calummackervoy deleted the calum/db-index-user-public-id branch July 9, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Ne doit pas figurer dans le journal des changements. python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants