Skip to content

Admin: Amélioration de la gestion des appartenances aux organisations#5595

Merged
tonial merged 2 commits into
masterfrom
alaurent/fix_memberships_admin
Feb 13, 2025
Merged

Admin: Amélioration de la gestion des appartenances aux organisations#5595
tonial merged 2 commits into
masterfrom
alaurent/fix_memberships_admin

Conversation

@tonial

@tonial tonial commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

🤔 Pourquoi ?

This way, we only allow it on the organization page.

We now need to handle the following issue : for organization with more than 250 members, we have more than DATA_UPLOAD_MAX_NUMBER_FIELDS limit of 1000.

We could make all the inlined memberships readonly ig there are too many and allow to change them from the user detail page ?
Or handle the memberships from a dedicated page that we filter from the url (?user=X or ?company=Y)

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?
  • Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@tonial tonial added the modifié Modifié dans le changelog. label Feb 11, 2025
@tonial tonial self-assigned this Feb 11, 2025
@tonial tonial force-pushed the alaurent/fix_memberships_admin branch 3 times, most recently from aefb8d1 to 9cb4a26 Compare February 12, 2025 07:17
@xavfernandez xavfernandez changed the title Admin: Amélioration de la gestion des appartenances aux organisaions Admin: Amélioration de la gestion des appartenances aux organisations Feb 12, 2025
@tonial tonial force-pushed the alaurent/fix_memberships_admin branch from 4ff8819 to 31cb638 Compare February 12, 2025 19:58
@tonial tonial marked this pull request as ready for review February 12, 2025 19:58
@tonial tonial force-pushed the alaurent/fix_memberships_admin branch 3 times, most recently from d9010a3 to 1bb2de6 Compare February 13, 2025 06:07

@francoisfreitag francoisfreitag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Comment thread itou/prescribers/admin.py Outdated
Comment on lines +80 to +82
if obj and obj.prescribermembership_set.count() > 200:
return False
return True

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

return bool(obj and obj.prescribermembership_set.count() > 200)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Je me demande si on veut vraiment laisser les admins toucher aux organisations de plus de 100 membres, j’ai du mal à croire qu’il ne reste plus aucun admin sur une organisation de cette taille. 🤷

Comment thread itou/prescribers/admin.py Outdated
memberships_reelname = "prescribermembership_set"
org_relname = "company"

def has_change_permission(self, request, obj=...):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

La signature est obj=None.

@francoisfreitag

Copy link
Copy Markdown
Member

Je pense qu’un petit test pourrait être utile, quitte à mocker le nombre de memberships pour éviter que le test ne soit trop coûteux.

This way, we only allow it on the organization page.
@tonial

tonial commented Feb 13, 2025

Copy link
Copy Markdown
Contributor Author

Je n'ai aucune idée de comment arriver à mocker le related manager obj.prescribermembership_set ^^'
Le test est un peu long en créant les 100 memberships (1.5 secondes)

@tonial tonial force-pushed the alaurent/fix_memberships_admin branch from 1bb2de6 to 988aca1 Compare February 13, 2025 09:33
@francoisfreitag

Copy link
Copy Markdown
Member

J’imaginais plutôt stocker la limite (100, 200, ton choix 😛) dans une constante et mocker la constante dans le test (pour mettre par exemple 2), et ainsi ne créer que 3 memberships.

For organization with more than 250 members, we go over the
DATA_UPLOAD_MAX_NUMBER_FIELDS limit of 1000.

Make the memberships readonly when they have too many (let supportix or
another member of the organization add the user)
We have less than 10 organizations with more than 200 members

No need to make this change for companies and institutions
since we have respectively at most 42 and 23 memberships
as of today.
@tonial tonial force-pushed the alaurent/fix_memberships_admin branch from 988aca1 to 8eea51f Compare February 13, 2025 11:52
@tonial tonial enabled auto-merge February 13, 2025 11:53
@tonial tonial added this pull request to the merge queue Feb 13, 2025
Merged via the queue into master with commit 6f6a749 Feb 13, 2025
@tonial tonial deleted the alaurent/fix_memberships_admin branch February 13, 2025 12:09
) in caplog.messages


def test_admin_more_than_100_memberships(admin_client, mocker):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hahaha, comme tu ne savais plus entre 100, 200 et 3, tu as mis 100 dans le titre et 2 dans le test ? 😂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oups...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

modifié Modifié dans le changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants