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

Model review and restructuring some models #21

Open
emrdagkusu opened this issue Dec 31, 2021 · 8 comments · May be fixed by #34
Open

Model review and restructuring some models #21

emrdagkusu opened this issue Dec 31, 2021 · 8 comments · May be fixed by #34
Assignees

Comments

@emrdagkusu
Copy link
Collaborator

emrdagkusu commented Dec 31, 2021

We should review our models on MongoDB to be able to make them proper for future developments. Also, we should add some models such as slots, absences.

class AbsenceModel(BaseModel):
    id: PyObjectId = Field(default_factory=PyObjectId, alias="_id")
    absence: str = Field(...)

    class Config:
        allow_population_by_field_name = True
        arbitrary_types_allowed = True
        json_encoders = {ObjectId: str}
        schema_extra = {
            "example": {
                "absence": "1,0,2",
            }
        }

For example in here instead of putting absences as comma-separated strings, we can have attributes and while decoding and encoding we can change them as we want and use them properly. And with this way, we can be able to control those attributes in the model instead of endpoints.

@emrdagkusu
Copy link
Collaborator Author

None of the string attributes of models has min_lenght or max_length validation. We should determine some constraints for the string values as we should do for numeric values. From the link below, you can see validation options for pydantic models, and then we can think, besides min_length and max_length, which options can be used.

https://pydantic-docs.helpmanual.io/usage/schema/#field-customization

@emrdagkusu
Copy link
Collaborator Author

emrdagkusu commented Jan 29, 2022

In the models there are some models (eg. UpdateEntranceYearModel, UpdateSemesterModel) that don't need _id value. We should remove those attributes.

Indeed, we should determine which attributes are required for the front-end and which are not. According to that, we can remove unnecessary attributes.

Also, we should consider the naming of attributes. Some of them have camel case (startDate, endDate, startHour) and some of them have initials (dLesson, dBreak). All of them should be consistent.

@emrdagkusu
Copy link
Collaborator Author

Now, there is no control for lesson names even if the given lesson name already exists. Should we allow duplicate lesson names?

@yusufuysal
Copy link
Collaborator

Now, there is no control for lesson names even if the given lesson name already exists. Should we allow duplicate lesson names?

I think that should not be allowed since users will not need this for any reason. Creating the same semester, lesson or absence can be a bug in our system.

The most appropriate response code to such request is debatable though.
https://stackoverflow.com/questions/3825990/http-response-code-for-post-when-resource-already-exists#:~:text=For%20the%20record%2C%20422%20is,a%20name%20already%20in%20use.
303 makes more sense to me.

@emrdagkusu
Copy link
Collaborator Author

emrdagkusu commented Feb 4, 2022

I suggest changing the name for "lesson" to "course". This seems more convenient.

Should we add another attribute called "grade" for lessons? If so, users can add their grades and they also can calculate their GPAs according to those grade values.

For university endpoints, there is no "create section" option. There is only a "create lesson" option and its behavior: if there is no lesson with the given code, it creates a new lesson and creates the first section with given attributes. If there is a lesson with a given code, it directly creates a new section. What are your ideas about this logic?

@hbusul
Copy link
Owner

hbusul commented Feb 4, 2022

Now, there is no control for lesson names even if the given lesson name already exists. Should we allow duplicate lesson names?

I guess we should not allow it. IMO 409 Conflict or 400 Bad Request seems like 2 good candidates for response.

@emrdagkusu
Copy link
Collaborator Author

emrdagkusu commented Feb 6, 2022

User lessons don't have an attribute called "code" unlike university lessons. I think we should also add the "code" attribute to user lessons. Also, "location" or "rooms" information would be useful.

Moreover, there is no relation between user lessons and university lessons. Therefore, if a lesson in the university lessons is updated, users cannot be aware of the lesson information changed. There should be a relation or at least we should notify users who have that lesson about the changes for that lesson.

@emrdagkusu
Copy link
Collaborator Author

Example university model that suggested by copilot:

university_data = {
            "name": "Test University",
            "domain": "test.com",
            "country": "USA",
            "state": "California",
            "city": "Los Angeles",
            "address": "123 Main Street",
            "zip_code": "90210",
            "description": "Test University Description",
            "logo": "test_logo.png",
            "cover_photo": "test_cover_photo.png",
        }

@emrdagkusu emrdagkusu linked a pull request Mar 17, 2022 that will close this issue
@emrdagkusu emrdagkusu linked a pull request Mar 17, 2022 that will close this issue
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 a pull request may close this issue.

4 participants