garg3133 / JagratiWebApp

Official WebApp of Jagrati - An Initiative of IIITians

https://jagrati.iiitdmj.ac.in

MIT License

47 stars 130 forks source link

Added subject taught field in CW/HW section #279

Closed satwik-talluri closed 3 years ago

satwik-talluri commented 3 years ago

Related Issue

Fixes: #176

Prosposed Changes

Added a select input field in 'Update CW/HW' section on the dashboard for marking which subject was actually taught in the class
The selected value in the select input defaults to the subject scheduled on that day
The selected value will be saved to the database only if the CW, HW and comment fields are not all empty
Displayed the Subject Taught alongside the Subject Scheduled field

Screenshots

Original	Updated

satwik-talluri commented 3 years ago

Hey @garg3133 , Please review this PR.

satwik-talluri commented 3 years ago

The UI part of the PR looks good. There are a few problems which I found in the working and implementation of this feature:

On selecting a subject from dropdown and submitting it, the subject is being updated in the database field subject_taught, but does not get updated at the top alongside Subject Scheduled.

There is absolutely no need to update the subject taught at the top in realtime as we select different values in the dropdown.

I don't understand why you passed the selected_subject as a context variable while accessing the dashboard. You could have just used {{ cw_hw.get_subject_taught_display }} to display the subject. (Display "Not Updated" in case the subject_taught field is None.

@garg3133 I have passed the selected_subject as a context variable to fetch all subjects for the subject_taught dropdown. Please let me know if there is any other way

garg3133 commented 3 years ago

I have passed the selected_subject as a context variable to fetch all subjects for the subject_taught dropdown. Please let me know if there is any other way

Ohh! But still, it's a really expensive block of code as it goes through all the schedules and modifies the dictionary each time.

You can easily get all the subjects from Schedule.SUBJECTS tuple (import Schedule model from models.py, if not already imported).

satwik-talluri commented 3 years ago

I have passed the selected_subject as a context variable to fetch all subjects for the subject_taught dropdown. Please let me know if there is any other way

Ohh! But still, it's a really expensive block of code as it goes through all the schedules and modifies the dictionary each time.

You can easily get all the subjects from Schedule.SUBJECTS tuple (import Schedule model from models.py, if not already imported).

@garg3133 Thanks, all the problems have been resolved. Please check if it is okay now.

garg3133 commented 3 years ago

Looks good. Merging it into master.

Thanks, @satwik-talluri for working on this.