France-ioi / AlgoreaBackend

Backend for the new Algorea platform
MIT License
1 stars 2 forks source link

Update group: handle approval requirement changes #1043

Closed GeoffreyHuck closed 5 months ago

GeoffreyHuck commented 5 months ago

fixes #1039

Implement rules when strengthening a `require_* field as detailed in #1039

Refactor of the test system

There are two limitations with the test system.

1. Everything is in one file, and it keeps growing

For now, the idea is to split it by modules:

It's a first step, it might be refactored into objects in the future because there are common functionalities between modules.

2. The way we handle default values

To provide a higher degree of abstraction, in many definitions, we define rows in tables that are a requirement of the definition (for example: when we had a member to a group, we also add a row in the user table).

The problem was that the system would put in database the values defined the last time if was defined, directly or indirectly. For example: if we define a group with name my name, but a later feature defines the same group with the name default name, the database would contain default name.

The solution retained is to explicitly declare in the testing system when we set a field:

  ctx.setGroupFieldInDatabase(
    ctx.getGroupPrimaryKey(groupID),
    "require_personal_info_access_approval",
    group["require_personal_info_access_approval"],
  )

In this way, values are not redefined at default when there are set.

This also makes it clearer what is defined in the current feature.

Test refactor

Documentation will be added in the devdoc to precise the plans. For now, it has only been made for groups, group pending requests and group membership changes, because those are the modules that were impacted with this PR.

Notes

The changes are large and since the commits are incremental and many refactoring were needed, it might be easier to review everything at once rather than commit by commit. But the commit messages contain all the details if needed.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (4ee3a9b) to head (1a496c9).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1043 +/- ## ========================================== Coverage 100.00% 100.00% ========================================== Files 234 236 +2 Lines 14059 14222 +163 ========================================== + Hits 14059 14222 +163 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GeoffreyHuck commented 5 months ago

There apparently a test coverage issue. I'll fix that tomorrow. But it's already reviewable.

GeoffreyHuck commented 5 months ago

The coverage is fixed, it was caused by code that cannot be reached. So I added a (rather dummy) test to reach it.