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

[frontend] Changing accessibility and registration data structure #985

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

AlexandreDoneux
Copy link
Contributor

@AlexandreDoneux AlexandreDoneux commented Nov 23, 2023

This pull request has as goal the change of accessibility and registration data structure in in the code as well as for the storage in DB and yaml files. The objective is also to store the dates as ISO dates in the database and yaml files. This concerns course accessibility, course registration period and task accessibility.

These accessibilities where previously a single value (accessible/registration for courses and accessibility for tasks) and were structured as follows :

  • a true when always accessible
  • a false when never accessible
  • a concatenated string containing all the dates separated by a "/"
    (ex: "2023-12-06 09:00:00/2023-12-13 09:00:00/2023-12-13 09:00:00")

We will now only use the dates and use minimal and maximal dates to define the cases where the the courses and tasks are always or never accessible. Those minimal and maximal dates are also used in a custom accessibility when certain dates are not defined. You can for example only define a start date in the form and INGInious will then assume there is no end date to the task and it will always be accessible after the given start date.

TO DO :

  • Change datastructure in AccessibleTime class and it's usage
    • adapt AccessibleTime
    • adapt usage basic INGInious app
    • adapt usage in contest plugin
    • Change AccessibleTime default parameters and it's use
  • Changing init structure
    • courses
    • tasks
  • Adapting forms to new structure
    • Transforming datetime objects into strings
    • course settings
    • task accessibility edit
    • task accessibility grouped edit
  • Dates as ISODate() objects in MongoDB
    • Storing (transform in datetime objects before storing in DB)
    • Retrieving
    • Correctly handling min and max dates
  • Storing dates as timestamps objects in yaml files
    • Storing
    • Retrieving
    • Correctly handling min and max dates
  • Automatically change structure in DB and yaml files when switching version
    • DB
      • [ ] In already existing collections => not needed
      • During "courses" collection init in legacy courses import
      • During legacy task import in task list edit
    • YAML files
      • During legacy task import in course template

@AlexandreDoneux AlexandreDoneux changed the base branch from master to tasksets November 23, 2023 15:57
@AlexandreDoneux
Copy link
Contributor Author

Rebase on cf8141c.

@AlexandreDoneux
Copy link
Contributor Author

Rebase on 1095603.

@anthonygego anthonygego changed the title [frontend] Changing accessibility and registration data structure + storing as ISO dates [frontend] Changing accessibility and registration data structure Jan 4, 2024
@nrybowski nrybowski deleted the branch UCL-INGI:master January 10, 2024 21:43
@nrybowski nrybowski closed this Jan 10, 2024
@nrybowski nrybowski reopened this Jan 10, 2024
@AlexandreDoneux AlexandreDoneux changed the base branch from tasksets to master January 11, 2024 14:46
@AlexandreDoneux
Copy link
Contributor Author

Rebase on 4de3a17.

Copy link
Member

@anthonygego anthonygego left a comment

Choose a reason for hiding this comment

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

I think at this stage that you can extract and movethe i18n changes to another PR so that this one gets clearer.

It seems there is no complete consistency for years "0001-" and "1-".

I would recommend, for the max date, to only work with a "one second" accuracy to avoid troubles between the code, the OS-specific accuracy and pymongo.

I eventually woner if keeping support for the older string format in AccessibleTime constructor could not make the code (and migration one) more cleaner.

inginious/common/custom_yaml.py Outdated Show resolved Hide resolved
inginious/common/custom_yaml.py Outdated Show resolved Hide resolved
inginious/frontend/accessible_time.py Outdated Show resolved Hide resolved
inginious/frontend/accessible_time.py Outdated Show resolved Hide resolved
inginious/frontend/accessible_time.py Outdated Show resolved Hide resolved
inginious/frontend/task_dispensers/util.py Outdated Show resolved Hide resolved
inginious/frontend/task_dispensers/util.py Outdated Show resolved Hide resolved
inginious/frontend/course_factory.py Outdated Show resolved Hide resolved
@AlexandreDoneux
Copy link
Contributor Author

Some code might need to be rewritten in settings.py :

def POST_AUTH(self, courseid): # pylint: disable=arguments-differ
""" POST request """
course, __ = self.get_course_and_check_rights(courseid)
errors = []
course_content = {}
data = flask.request.form
course_content = self.course_factory.get_course_descriptor_content(courseid)
course_content['name'] = data['name']
if course_content['name'] == "":
errors.append(_('Invalid name'))
course_content['description'] = data['description']
course_content['admins'] = list(map(str.strip, data['admins'].split(','))) if data['admins'].strip() else []
if not self.user_manager.user_is_superadmin() and self.user_manager.session_username() not in course_content['admins']:
errors.append(_('You cannot remove yourself from the administrators of this course'))
course_content['groups_student_choice'] = True if data["groups_student_choice"] == "true" else False
if data["accessible"] == "custom":
course_content['accessible']["start"] = datetime.strptime(data["accessible_start"], '%Y-%m-%d %H:%M:%S') if data["accessible_start"] != "" else course._accessible.min
course_content['accessible']["end"] = datetime.strptime(data["accessible_end"], '%Y-%m-%d %H:%M:%S') if data["accessible_end"] != "" else course._accessible.max
elif data["accessible"] == "true":
course_content['accessible']["start"] = course._accessible.min
course_content['accessible']["end"] = course._accessible.max
else:
course_content['accessible']["start"] = course._accessible.max
course_content['accessible']["end"] = course._accessible.max

The data of the course is accessed through the course_factory :

course_content = self.course_factory.get_course_descriptor_content(courseid)

However, the course is retrieved at the start of the method. We could rewrite a bit to use the course instead of self.course_factory.get_course_descriptor_content(courseid).

There is also the use of a protected member to use the min and max dates instead of datetime.min and datetime.max.replace(microsecond=0). A method on course retrieving the content would allow us acces to those values.

@AlexandreDoneux
Copy link
Contributor Author

AlexandreDoneux commented Apr 18, 2024

Keeping support for the legacy access format and changing the format on saving the settings of a course or task is not as easy as we thought for the moment.

As you suggested we can add support in the AccessibleTime init. For a course accessibility and registration period it only needs some tweaks. However task accessibility is more difficult as the accessibility of a task for the edit pages (taskset template and course tasks) is not passed through AccessibleTime.

For example in toc.py, task_config is passed to the jinja template and contains the data of the different tasks.

    def render_edit(self, template_helper, element, task_data, task_errors):
        """ Returns the formatted task list edition form """
        config_fields = {
            "closed": SectionConfigItem(_("Closed by default"), "checkbox", False),
            "hidden_if_empty": SectionConfigItem(_("Hidden if empty"),"checkbox",False)
        }

        taskset = element if isinstance(element, inginious.frontend.tasksets.Taskset) else None
        course = element if isinstance(element, inginious.frontend.courses.Course) else None
        task_config = dict_data_datetimes_to_str(self._task_config)

        return template_helper.render("task_dispensers_admin/toc.html", element=element, course=course, taskset=taskset,
                                      dispenser_structure=self._toc, dispenser_config=task_config, tasks=task_data,
                                      task_errors=task_errors, config_fields=config_fields)

However, when tracing back from where the data comes from we can see it is retrieved from the database here, in course_factory.py, and is never processed by AccessibleTime :

    def get_course_descriptor_content(self, courseid):
        return self._database.courses.find_one({"_id": courseid})

Edit:
I changed the access structure change to be applied when importing legacy tasks. The has_legacy_tasks() method has been modified to detect if tasks have the old access structure. The legacy tasks import button will be displayed if it's the case (in task_list.html and template.html).

Adding an "accessible_period" dict to define start and end period.
… accessibility and registration

Courses will now use a dictionnary for "accessible_period" and "registration_period" with a start and end element. Tasks will have an "accessibility_period" dict with an extra soft_end element.
Putting boolean and period dict inside one element : "accessibility": {"is_open": bool, "period": {"start": ..., "soft_end": ..., "end": ...}}. This way dispenser_data["config"] init in TableOfContents does not have to be changed.
…nd DB

To keep the same structure as the task accessibility (dict with bool and period dict).
…enser

Changing access structure of tasks when importing in course template and task edit. Also moving change_access_structure, dict_data_str_to_datetime and dict_data_datetimes_to_str in util file in frontend directory.
+ course_factory method used on taskset_factory
…nto strings

Had years with less than 4 digits not transformed into strings with years without leading "0" characters.
Simplifying 4 digit year check + removing empty string check because can only be applied to datetime objects
…seconds

Storing as min and max attributes of AccessibleTime when possible. And using dates without microseconds
by adding boolean, empty string and None values
…stration date

+ fix None value and empty string
…tasks settings update

Previously done when importing legacy tasks
@AlexandreDoneux
Copy link
Contributor Author

Rebase on b3a4095.

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