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

Team page #149

Open
wants to merge 42 commits into
base: main
Choose a base branch
from
Open

Team page #149

wants to merge 42 commits into from

Conversation

yukitya-1811
Copy link
Contributor

Description

Add Team Page

  • Adding a team page to display faculty advisors, core members and executive members of all SIGs (including CompSoc, Diode, Piston and ExeCom)
  • Adding Core and Faculty model in accounts/models.py

(Closed PR for reference - #134)
(Closed PR for reference - #141)
Fixes #139

Type of Change

What types of changes does your code introduce?
Put an x in the boxes that apply

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

yukitya-1811 and others added 30 commits May 8, 2024 23:52
… Members. Also added proper sequential loading of Core Members
Co-authored-by: Nishant Nayak <[email protected]>
This reverts commit 829451d, reversing
changes made to 61aba1f.
Copy link

render bot commented May 8, 2024

corpus/accounts/models.py Outdated Show resolved Hide resolved

class Core(models.Model):

POST_CHOICES = (
Copy link
Member

Choose a reason for hiding this comment

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

As discussed on call, any reason for this? I think it'll be better to make another model or something called Post and have a ForeignKey linking to that table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, would the mapping remain the same for the core posts then? Like rn I'm having a POST_CHOICES which maps every core post to a number, according to which the core posts are fetched and displayed. So can I move this exact logic to the Post model when I create it?

class Meta:
verbose_name_plural = "faculties"

FACULTY_POSTS = (
Copy link
Member

Choose a reason for hiding this comment

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

This could also go into the Post model. Then no need to differentiate by like faculty posts and stuff

Copy link
Member

Choose a reason for hiding this comment

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

Nope. I would like Faculty to be in a different model. Core members have to be Executive Members, Faculty will only be Users. The foreign key references will be different.

Also, access control in the future will be much easier if we have these models separate.

corpus/impulse/views.py Show resolved Hide resolved

def team(request):
compsoc_members = ExecutiveMember.objects.filter(
(Q(core__isnull=True) | Q(core=None))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you've made ExecutiveMember.core as an association right? I think this is what this query does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made a Core.executivemember relation. Again since every Core member has a user model associated, i'm just checking that we do not fetch such users

def team(request):
compsoc_members = ExecutiveMember.objects.filter(
(Q(core__isnull=True) | Q(core=None))
& (Q(user__faculty__isnull=True) | Q(user__faculty=None))
Copy link
Member

Choose a reason for hiding this comment

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

User does not have faculty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No but faculty can have a user, right? So I'm just checking that the user I'm fetching does not have a faculty linked to it

& (Q(user__faculty__isnull=True) | Q(user__faculty=None))
& Q(sig__name="Piston")
)
ieee_core = Core.objects.filter(sig__name="ExeCom").order_by("post")
Copy link
Member

Choose a reason for hiding this comment

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

We can't add ExeCom as a SIG, it is both wrong logically, and will mess up both current and future code

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure how else to identify ExeCom members then.
As Executive Members, their SIG will only be one of CompSoc/Diode/Piston.
As Core Members, they can only be part of CompSoc/Diode/Piston/SAC/ExeCom. All these 5 categories have to be displayed separately.

Maybe sig isn't the best name for the attribute in the Core model, but this is logically correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I have done in the newest changes is include 2 attributes. is_execom and is_sac which helps us pick out executive committee and sac members without compromising their actual sigs

& (Q(user__faculty__isnull=True) | Q(user__faculty=None))
& Q(sig__name="CompSoc")
)
diode_members = ExecutiveMember.objects.filter(
Copy link
Member

Choose a reason for hiding this comment

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

Two things you can do to solve this:

  1. Duplicate core members in the members list. This is fine, as although they are in core, they are still members, so should be okay.
  2. Get all the core posts, and then remove those from all the members in that SIG.

Also, you'll have to make sure that you query for this particular year. You can probably make a configuration where you define what the current academic year is, and then use that for queries to get "current" people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I like check if their term_end is null or not? If term_end is null, then display, otherwise don't?

@anirudhprabhakaran3
Copy link
Member

I think we'll have to sit and rewrite the team page view function to make sure we cover all edge cases and stuff.

@nishant-nayak nishant-nayak self-requested a review May 27, 2024 15:33
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