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

Allow users to apply for yearbook class #25

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

Conversation

MarioRodrigues10
Copy link
Member

No description provided.

alias Yearbook.University.ClassStudent

def create_class_student(attrs \\ %{}) do
aluno = get_all_classes_students(attrs["student_id"], attrs["class_id"])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
aluno = get_all_classes_students(attrs["student_id"], attrs["class_id"])
student = get_all_classes_students(attrs["student_id"], attrs["class_id"])

Comment on lines 304 to 305
if aluno > 0 do
{:error, %Ecto.Changeset{}}
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 get this error. Can you explain it?
Also, you should send a message when throwing it, not a changeset.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm verifying if a student is already in a class

Copy link
Member

Choose a reason for hiding this comment

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

Just simply add a database constraint then

end
end

def get_all_classes_students(student_id, class_id) do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_all_classes_students(student_id, class_id) do
def list_classes_students(student_id, class_id) do

|> Enum.count()
end

def get_classes_students(student_id) do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_classes_students(student_id) do
def list_student_classes(student_id) do

@@ -19,4 +42,10 @@ defmodule YearbookWeb.YearbookLive.Show do
|> assign(:page_title, class.degree.name)
|> assign(:class, class)}
end

def is_accepted?(student, class) do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def is_accepted?(student, class) do
defp is_accepted?(student, class) do

@ruilopesm
Copy link
Member

We should discuss if this is the better approach. Another approach that I can think of would be the course director supplying us the classes' students.

@MarioRodrigues10
Copy link
Member Author

MarioRodrigues10 commented Apr 7, 2023

We should discuss if this is the better approach. Another approach that I can think of would be the course director supplying us the classes' students.

I also agree with that, but that helps us verify if a student is a part of a yearbook class or not.
Basically in my point of view, a student apply for a yearbook class, and then we can accept or deny based on if he's or not part of that of class.

@ruilopesm
Copy link
Member

But how would we know if the student is part of the class? If we are using the data given by the course director, shouldn't we make a script to seed the database and it's done?
@ruioliveira02 what's your opinion on this?

@MarioRodrigues10
Copy link
Member Author

We will receive an excel sheet from course director with the necessary data and our task would be to cross-check if the users who applied are present in the sheet. We cannot automatically populate the database as we are unsure of the participants preferences, and even if everyone desires to be featured in the yearbook, also we don't know their application accounts.

Additionally, I am developing this feature in a separate repository with the intention of eventually migrating it to the atomic repository.

def create_class_student(attrs \\ %{}) do
student = list_classes_student(attrs["student_id"], attrs["class_id"])

if student > 0 do
Copy link
Member

Choose a reason for hiding this comment

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

These types of verifications should be at the database level. Otherwise, you can not guaranty it.

Just make sure you have the unique index of [:student_id, :class_id] 💪

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