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

Added archived teams page. #1741

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

Conversation

sarahboyce
Copy link
Contributor

Added a basic archive of teams and way to record former team members based off this forum discussion

Screenshots

Archive page - no data

Archive page - some data

Team page link

@sarahboyce sarahboyce marked this pull request as ready for review November 16, 2024 11:09
@bmispelon
Copy link
Member

Thanks for working on this!

About the model design, did you consider a custom through model for the Team.members m2m that would have an active boolean field?

I can see that the proposed design with former_members is probably simpler to implement, but it seems to me that it leaves the possibility for some strange limbo cases.
Consider an archived team with active members: unless I misunderstood those members would not be listed on the new archive page.
There's also nothing stopping a member being both in members and in former_members, was that an intended design?

@sarahboyce
Copy link
Contributor Author

Thanks for working on this!

About the model design, did you consider a custom through model for the Team.members m2m that would have an active boolean field?

I can see that the proposed design with former_members is probably simpler to implement, but it seems to me that it leaves the possibility for some strange limbo cases. Consider an archived team with active members: unless I misunderstood those members would not be listed on the new archive page. There's also nothing stopping a member being both in members and in former_members, was that an intended design?

I can do this
There might be people who want to show their former membership on the Steering Council and be a current member but I can acheive a similar thing and have their details be on the team page if they want a comment by their name

@tim-schilling
Copy link

There's also nothing stopping a member being both in members and in former_members, was that an intended design?

That seems reasonable to me. It's easier to manage having a person appear in both membership groups with two M2M fields.

To handle the limbo cases, a team could have some validation that when active is set to False, it confirms there are no members. Or clears the membership.

@sarahboyce
Copy link
Contributor Author

About the model design, did you consider a custom through model for the Team.members m2m that would have an active boolean field?

I can do this

Before I do the work here, as you cannot add a through model to an existing M2M field, I would need to create a new m2m, migrate the data, remove old field, rename field etc etc
Is it worth it?

@sarahboyce
Copy link
Contributor Author

sarahboyce commented Nov 16, 2024

Consider an archived team with active members: unless I misunderstood those members would not be listed on the new archive page.

They are listed, see alice in test_archived_team

@bmispelon
Copy link
Member

In light of the fact that:

  1. I was wrong about active members not being displayed for archived teams;
  2. Having someone be listed both as an active and archived member of a team is a desired feature;
  3. Changing an m2m relationship to an explicit through model is a lot of busywork;

Then this approach seems like the best option 👍🏻 .
I've reviewed the PR and it's good to go ✅ , but we've since merged translations for both members and the whole project, so the PR needs to be updated to have translatable strings everywhere (which would fix the current merge conflicts too).

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