search

TEAMMATES / teammates

This is the project website for the TEAMMATES feedback management tool for education
https://teammatesv4.appspot.com/
GNU General Public License v2.0
1.67k stars 3.3k forks source link

CoursesLogic: Push soft-delete logic to DB layer #9124

Open dalessr opened 6 years ago

dalessr commented 6 years ago

Environment: master branch at commit https://github.com/TEAMMATES/teammates/commit/c0f92b439da732fc4ec3a4e18c9133c90868531f

Description: Current implementation for course's Recycle Bin feature: when moving the course to Recycle Bin, no item inside will be changed (i.e. sessions, instructors, students). However, Since some sessions inside are still active & open, thus the sessions can still be reached from logic layer, even though the course has been soft-deleted.

Solution: When soft-deleting courses, also move all the corresponding sessions to Recycle Bin so that logic layer cannot reach them. When restoring the course, also restore the sessions that are deleted after the time the course has been deleted.

xpdavid commented 6 years ago

When soft-deleting courses, also move all the corresponding sessions to Recycle Bin

This will need data migration. You may want to introduce the logic in a separate PR.

The urgent task is to disable "session which is not in recycle bin but its corresponding course is in recycle bin can still be accessed". i.e. Introduce getCourse() and getCourseFromRecycleBin()

dalessr commented 6 years ago

Ok so assuming that a soft-deleted session is actually "disabled", then for the current/upcoming PR I will only soft-delete the sessions without modifying the logic. Also, those soft-deleted sessions wont be shown in session's Recycle Bin as it always checks whether the corresponding course has been deleted.

niqiukun commented 4 years ago

Soft-deleting all sessions when soft-deleting the course will cause an issue because if some of the sessions of a course are soft-deleted before the course, soft-deleting the course will make all sessions soft-deleted regardless, so after recovering the course it is not possible to restore the sessions to their previous status. What about checking the student's accessibility to courses first when checking their accessibility to sessions?

xpdavid commented 4 years ago

Soft-deleting all sessions when soft-deleting the course will cause an issue because if some of the sessions of a course are soft-deleted before the course, soft-deleting the course will make all sessions soft-deleted regardless, so after recovering the course it is not possible to restore the sessions to their previous status.

Good point. Yup, I remember this was the initial consideration at that time.

What about checking the student's accessibility to courses first when checking their accessibility to sessions?

It will work. There is indeed a check in the code for this case for another scenario.

https://github.com/TEAMMATES/teammates/blob/59c356e84ccfbf5863797fb5ca80270be506ff41/src/main/java/teammates/logic/core/FeedbackSessionsLogic.java#L1024-L1044

Personally, I don't like the solution as this require any future feature involving getting feedback session to do a prior check of course. Developers will miss this easily.

wkurniawan07 commented 4 years ago

The only reliable way to solve this issue is to have another flag in the feedback session itself for whether the corresponding course is deleted

vigneshreddy17 commented 7 months ago

Hi, is this issue still open? I can take a look at it.

cedricongjh commented 7 months ago

hi @vigneshreddy17 this issue is part of our V9, and meant for committers only, we encourage you to take a look at other issues to contribute to!