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

Feat/search bar/backend #472

Open
wants to merge 6 commits into
base: feat/coord-interface/main
Choose a base branch
from

Conversation

ElaineShu0312
Copy link

Implemented search.py, which has the following functionality:

What we can search for, with "query" or "student_absences":

  • student first/last name
  • mentor first/last name
  • student email
  • mentor email
  • student absences (range or exact)

What it should return:

  • all students that match the query
  • the associated sections that have the students in them

E.G. if you query student_absences=4, the JSON will look like:

        {
            "id": 91,
            "spacetimes": [
                {
                    "startTime": "22:48:46.548833",
                    "dayOfWeek": 4,
                    "location": "Moffitt 300",
                    "id": 135,
                    "duration": 3600.0,
                    "override": null
                }
            ],
            "mentor": {
                "id": 91,
                "name": "Matthew Garcia",
                "email": "[[email protected]](mailto:[email protected])",
                "section": 91
            },
            "capacity": 4,
            "associatedProfileId": 2,
            "numStudentsEnrolled": 4,
            "description": "",
            "course": "EECS16B",
            "userRole": "COORDINATOR",
            "courseTitle": "Designing Information Devices and Systems II",
            "courseRestricted": true
        }
    ],"students": [
        {
            "id": 224,
            "name": "Grant Hudson",
            "email": "[[email protected]](mailto:[email protected])",
            "section": 91,
            "mentor": "Matthew Garcia",
            "numAbsences": 4
        }
    ]```
    

Copy link
Member

@smartspot2 smartspot2 left a comment

Choose a reason for hiding this comment

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

Overall, seems good for an initial implementation of a search query, with a few small comments. I'd forsee that you'll need to create a new custom serializer for this data though, since any expansions/modifications to the kinds of data we're querying over would be hard to do with this current implementation.

models.dot Outdated
Copy link
Member

Choose a reason for hiding this comment

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

The model visualization files should be excluded from the commits (either move them outside of the repository folder, or delete them)

@@ -23,4 +23,7 @@
path("matcher/<int:pk>/mentors/", views.matcher.mentors),
path("matcher/<int:pk>/configure/", views.matcher.configure),
path("matcher/<int:pk>/create/", views.matcher.create),
path(
"search/get-sections/", views.get_sections_of_user, name="get_sections_of_user"
Copy link
Member

Choose a reason for hiding this comment

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

Generally, the path itself shouldn't need to include actions like "get"; the HTTP method that the request has should indicate what the user wants. The more conventional path would be something like search/sections/.

@api_view(["GET"])
def get_sections_of_user(request):
"""
Gets all sections that match the queried user.
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend adding a little more detail in this docstring describing exactly what the view expects from the request (ex. all of the possible query parameters, possible values for each parameter, how it handles each parameter, etc.)

Comment on lines 51 to 57
students = Student.objects.filter(
# pylint: disable-next=unsupported-binary-operation
Q(user__first_name__icontains=query)
| Q(user__last_name__icontains=query)
| Q(user__email__icontains=query),
section__in=sections,
).distinct()
Copy link
Member

Choose a reason for hiding this comment

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

This students object seems to be overwritten in the if statements below when working with student_absences. If it's possible to provide both the query and the student_absences parameters, then they should work together properly. (And this should be explained in the docstring as well.)

return Response(
{
"error": (
"Invalid value for student_absences. Please provide an integer."
Copy link
Member

Choose a reason for hiding this comment

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

It seems like you also accept ranges, so more than just integers should be expected (and the error message should reflect that).

An alternative to providing this union of possible types is to specify a new (mutually exclusive) query parameter for a range of student absences. Either works though, so it's up to you whether you want to stick with this or split it up.

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.

2 participants