france-ioi / algoreabackend Goto Github PK

Remove schema_revision and synchro_version tables.

The way partial access is propagated from an item to his child is defined is items_items.bAlwaysVisible and items_items.bAccessRestricted. In addition to be not very clear, they allow expressing 4 "cases" while there are actually 3 cases.

So, convert items_items.bAlwaysVisible and items_items.bAccessRestricted to one single attribute items_items.partial_access_propagation of type ENUM('none','as grey','as partial').

Map the existing data:

• ! restricted -> 'as partial'
• restricted & always_visible -> 'as grey'
• restricted & !always_visible -> 'no'

Motivations

Now the new permissions are implemented, use a more appropriate permission rule on who can set additional time.

Subtasks

In contestAdminList and ContestManagedByUser function (so contestGetGroupByName, contestListMembersAdditionalTime, contestSetAdditionalTime), implement that the one who can administrate a contest ....

? that the one who can modify the additional time are those who can distribute it.
? can watch on the group by which the user has access to the contest

Currently, feature files repeat for each scenario the seed to the db, make it definable once for the whole scenario

In a nutshell, lockUserDeletionDate is supposed to be used to prevent a user from deleting his account to participate several time to a contest.
This support should be fully implemented by putting it both in the user deletion (already done) but also to prevent a user to leave these groups.
Probably this attribute should be renamed to a more appropriate naming (e.g., user_membership_locked_until)

In order to do hierarchy checkings for items fast we need to add an index in 'items_items' table on (idItemParent, idItemChild) or on (idItemParent, idItemChild, iChildOrder)

Update the existing contestSetAdditionalTime service.
For all groups in the contest's items.contest_entering_group_id, update their group membership expiration to the new contest_started_at + contest_duration + total additional_time

Quick task: Could you add a comment for the tables groups_contest_items and contest_participations you have added recently?

From Go 1.11, Go supports semver for dependencies. See https://github.com/golang/go/wiki/Modules

To do:

• generate go.mod go.sum for the project
• update Makefile, .circleci/config.yml and README.md, if needed, accordingly

Everywhere in the code we have error messages starting with capital letters. At the same time, the “Go Code Review Comments” article recommends start them with lowercase letters (https://github.com/golang/go/wiki/CodeReviewComments#error-strings).

To keep HTTP errors starting from uppercase letters we should modify code

    return &ErrorResponse{
        Response:  response,
        ErrorText: e.Error.Error(), // FIXME: should be disabled in prod
    }

in service.httpResponse() to uppercase the first letter. After doing this we can follow the rule without modifying tests.

I tried to apply our migrations on the loaded prod db on MySQL8 and it fails on some aspect of the schema and data (which is probably invalid in MySQL8).

1. I assume the schema we have does not match the prod one (strange, I don't know why), so it would need to be fixed
2. Some data (such as "0000-00-00 00:00" datetime) are not valid in MySQL8 and make the migrations fail. Probably it requires to add a first migration before all to fix these.

I had issues on migrations n°2, n°14 (for which I had to do UPDATE history_groups SET sDateCreated = NULL WHERE sDateCreated < '1000-01-01'), n°17, ...

Edit:
a) The new prod export (from 22/09) should be emptied and considered as the the new "base schema"
b) Then, we have to see what migrations fails with this schema (fixing error of type (1), see above)
c) Then, we have to see what may fail in prod by relaunching the migration and prepend a migration fixing data (fixing error of type (2), see above)

Make sure to do (a), (b), and (c) in this order and in different commits.

As seen in the StructuredLogger middleware, the three following middlewares must be loaded in this specific order:

1. RequestID
2. StructuredLogger
3. Recoverer

Otherwise, they cannot do what they are supposed to do. It would be also the opportunity to test this part of app.go.

Contest management will be handled through expiration on group memberships (cfr contest explanation).
So let's first handle expiration without contest.
Let's consider the existing services do not need to set or get this expiration, but membership is only considered as actual if the expiration is not past.
Maybe to make this cleaner in regards of the existing statuses, it would be interesting to invent a new "contestEntered" (or just "system"?) membership, which would be the only one with this expiration for the moment.

Currently, when run make test it executes BDD tests twice which makes testing slow. It would be much better to execute them just once.

• Get rid of contest finish date in every table
  • For former contests:
    • create a contest_participants_group_id group for each contest
    • put the participants (teams or users-not-participating-as-a-team) in this group with a expiration matching their finish date
    • remove their partial access to the contest (the same way it was done before) if they still have
  • All functions around checkSubmissionRightsForTimeLimitedContest/getActiveContestInfoForUser should use the membership expiration (so close*Contest fcts do not make sense anymore)

When an item is created with a duration, create the participant group (with a new type "ContestParticipants", not allowed as input type from these service) as contest_participants_group_id.
Give this group the partial access on the item.

On edit:

• If a duration is added and contest_participants_group_id is NULL, do the same.
• If the duration is set to null, do nothing. (keep the group)

Update the comment of items.contest_participants_group_id to "Group to which all the contest participants (users or teams) belong. Must not be null for a contest."

In the new definition of the info level (previously grayed), a user with "info" access on an item can see the description, subtitle of the item.
itemView has to be adapted to reflect this change.

The end goal will be to implement the group permissions as described in https://france-ioi.github.io/algorea-devdoc/design/access-rights/groups/ (and especially at first, the db schema on the top)

Step 2: Introduce group pending requests
Description:
The new "group pending requests" table will be used to store the requests which requires the action from a user (group owner/manager or member). It will allows to get rid of all membership states corresponding to yet-to-be-approved and so non-actual memberships in the groups_groups table.

Subtasks:

• Add the group_pending_requests table with attributes:
  • group_id [PK, FK]
  • member_id [PK, FK]
  • type [enum: invitation, joinRequest]
  • at
• Move the 'invitationSent', 'requestSent' groups_groups entries to group_pending_requests
• Adapt the existing services so that they store, check, remove pending request / invitation in this table.
• Remove the groups_groups.type and groups_groups. type_changed_at
• Add content from group_pending_requests in "currentFullUserDataExport" service

Rename the ‘descendants_and_solution_view_propagation’ column to match naming in from the doc.

• Move contest_participations .entered_at to the attempts table (as contest_entered_at) as it is related to attempt and not to the group in general.
• Delete the contest_participations table as it is empty now
• Update the services

Hi,

The field groups.sPassword can be NULL to indicate that a group cannot be joined by password. However, AlgoreaBackend doesn't allow to set this field back to NULL once a password has been set ; the endpoint /groups/{group_id}/change_password only allows for a new password to be generated.

Would it be possible to add a way to set this field to NULL? (maybe through the /groups/{group_id}/change_password endpoint)

Thanks!

The table groups_items_propagate has been created to put away of groups_items the attribute sPropagateAccess, in particular to avoid changing groups_items too often, which run a trigger and store more entries.

However sPropagateAccess is still stored in both groups_items and groups_items_propagate and still groups_items.sPropagateAccess is still used in the propagation algorithm while it should not.

Remove the usage of groups_items.sPropagateAccess in the "compute all access" propagation (without breaking it 😁) and remove the attribute from groups_items.

see specs

Initially only users_items were used. Then groups_attempts were added to both implement participation in teams and allow several attempts for the same task.
We now we want to go towards handling a user the same way as a team; and all participations as attempts (even if most items would only allow one attempt at first).

So, we'll merge most of users_items data into groups_attempts. In practice, we will keep the users_items rows (as some of the data is still relevant for now) but the following columns will disappear (they are all in groups_attempts already):

score
score_computed
score_reeval
score_diff_manual
score_diff_comment
submissions_attempts
tasks_tried
tasks_solved
children_validated
validated
finished
key_obtained
tasks_with_help
hints_requested
hints_cached
corrections_read
precision
autonomy
started_at
validated_at
finished_at
latest_activity_at
thread_started_at
best_answer_at
latest_answer_at
latest_hint_at
contest_started_at
ranked
all_lang_prog

(actually but the ids, state/answer and platform_date_removed)

For the data migration:

1. For users_items without active_attempt_id, they have worked as a single user (no team) and have no groups_attempt for themselves on this item. For these, create a groups_attempts with the same data + the group_id being the user's self group + order '1'.
2. For users_items with an active_attempt_id, typically it is a copy of the data should already be in groups_attempts (there are scores not matching but that's ok to loose them); so we can remove the attributes in users_items without doing anything.

The users_items.platform_date_removed attribute can be removed as well, it has not been used yet, and we'll find a more appropriate solution later.

The ancestors_computation_state used for propagation of the scores does not make sense anymore.

Finally, the "state" and "answer" attribute are just a duplicate from what is in users_anwers and can be deleted as well.

Implement new permission rules and propagation described in https://france-ioi.github.io/algorea-devdoc/design/access-rights/items/

Various remarks:

• There are no more "*_from" date for permissions. For the existing data, they are all with date in the past, so they should just be converted to their value. The date can be used for setting the latest_update_on.
• Map gray, partial, full, solution access to respectively info, content, content_with_descendants and solution for "can_view".
• owner access on items become "is_owner"
• manager access in items become can_view=solution, can_grant_view=solution, can_watch=answer, can_edit=all, is_owner=false
• sAccessReason can be lost
• idUserCreated become owner_group_id (through their self_group_id)
• For existing data, set content_view_propagation to the partial propagation, view_propagation_limit to solution, grant_view_propagation to true, watch_propagation to true, edit_propagation to true
• groupItemEdit needs major updates. In addition to implement the major rules, it also have to take a new "group_id" input : group as which the user is doing the change.
• create a group named "Former Task Owner" with all task owners in it, and give this group "can_edit" = "children" to all items having items.bCustomChapter = true
• [NEW] remove all propagation from the current custom chapters to their direct children
• items.bCustomChapter can be removed

We agreed that these history tables are both hard to maintain and a potential performance issues (for migrations and mass update/delete). We will achieve the same objective with better event logs, sql query logs (provided by RDS) and point-in-time restoration (provided by RDS).
So let's remove all history_* tables, their triggers and the version attributes on all tables.

The end goal will be to implement the group permissions as described in https://france-ioi.github.io/algorea-devdoc/design/access-rights/groups/ (and especially at first, the db schema on the top)

Step 1: Introduce group_membership_changes
Description:
The new "group membership changes" table will be used to store the history of changes to memberships, i.e., group-group relationship between any group and a member (only users for now, maybe teams later). It will allows to get rid of all membership states corresponding to former memberships and better track the former states of a membership (not only the last one).

Subtasks:

• Add the group_membership_changes with attributes:
  • group_id [PK, FK]
  • member_id [PK, FK]
  • at [PK] (time of the action)
  • action [enum: invitation_sent | invitation_withdrawn | invitation_refused | invitation_accepted | join_request_send | join_request_withdrawn | join_request_refused | join_request_accepted | left | removed | joined_by_code | added_directly | expired]
  • initiator_id (FK, nullable): the user at the source of the action (if any), typically of group owner/manager or the user himself
• Copy the non "direct" groups_groups entries to group_membership_changes (you can put the user himself as initiator for actions initiated by the user, inviting_user_id for invitation creation)
• Remove from groups_groups entries with the 'invitationRefused', 'requestRefused', 'removed' and 'left' types
• Adapt the existing services
  • So that they store the membership changes
  • Do not store groups_groups with 'invitationRefused', 'requestRefused', 'removed' and 'left' types in `groups_groups.
  • Consider 'invitationRefused', 'requestRefused', 'removed' and 'left' types in existing services as the same non-existing membership state
  • In particular:
    • groupInvitationReject: do not check anymore that it was already 'invitationRefused'
    • groupsMembershipHistory: use the group_membership_changes table (maybe change type_changed_at to at only?)
  • invitationsView: for the *Refused types "within_weeks", use group_membership_changes
  • groupRequestsView: for the *Refused types within "old_rejections_weeks", use group_membership_changes
• Remove groups_groups.inviting_user_id
• Add content from group_membership_changes in "currentFullUserDataExport" service

Integrate Swagger to the API so that an API doc can be run (document it rather than automate the doc generation part)

Add a command to delete all temp users with expired sessions.
Remove the related entries (user self groups and co). See https://github.com/France-ioi/AlgoreaPlatform/blob/master/shared/RemoveUsersClass.php#L76

Remarks:

• It should be tested
• It should be easy to write a service for the same thing later.
• Soon, we'll have to be able to delete other kind of users, so make it modular (cfr AlgoreaPlatform).

So remove it from the input of the service.

About the "groups_attempts.minus_score" attribute which was added to make index efficient:

• Is it still required in MySQL8?
• Could we do it a generated column so that it is not forgotten when updating data?
• This can probably even be a virtual column (not stored) as they can be used for indices.
• In any case, if not deleted, this column should have a comment in the db, I was not remembering what this column was about.

For the moment, only a configuration entry which has been defined in the config yml file (whatever its value) can be defined as an ENV variable. If not in the yml, the env var is not retrieved.

groups.PasswordEnd is defined as "When will the password expire? (set when it's first used)". So maybe it should not be modifiable via the API? It should be set by the backend when the password is used?

In all tables that we use in Go (so not users_threads, ...), remove the prefix of attribute names and convert the camel case to snake case.
So:

• remove the "s", "i", "b" prefixes
• "idAbc" has be converted to "abc_id"
• tranform camel to snake case

Some examples:

• sName -> name
• sGradeDetails -> grade_details
• idItem -> item_id
• bIsSelf -> is_self
• idTeamItem -> team_item_id

Do not let the requester set a very huge limit (the limit parameter) as it may affect performance and be a base a DoS attack. Max the given value with a fixed value (5000?).

In app/api/answers/get_answers.go, the spec does not mention 404 while the service may return 404.

Most "datetime" attributes in the db are called "xyz_date" while it is not just a date.
I think "xyz_datetime" is not appropriate as the goal is not to put the type in it but just to have to have something semantically correct.
IMO, "_time" is correct (even if it is not a mysql "time" as well), as a time semantically may be a precise date.
But in most of the time, the attribute naming should not require any suffix, "partial_access_from" would be more appropriate.

BTW, for the durations, I would prefer having them in decimal seconds instead of using the MySQL time format.

Let me know what you plan to do before starting.

Try to upgrade from MySQL to MySQL 8.0 and run the full migrations + tests.

Expected achievements:

• IF the upgrade requires some data or schema migration before, create the migration script or query under db/.
• IF it requires some data or schema migration after (because some tests do not pass), make sure to add them (may be required to put it as first migration)
• Make sur the docker-compose and circle-ci config are correctly updated

"Root" groups are special groups which are defined in the config and set as the initial group of some new users (may be specific for temp users, or admin, ...). These groups are web-domain specific and so config should define the root groups for each served domain. (and probably reject not configured domains?) (so should probably managed in a middleware)

In app/api/auth/create_temp_user.go:82, the groupID in which temp user has to be put should be retrieved from the config. (so no retrieval based on sType, sName, or `sTextId)

The groups.sType enum('Root','Class','Team','Club','Friends','Other','UserSelf','UserAdmin','RootSelf','RootAdmin') has be to changed to enum('Class','Team','Club','Friends','Other','UserSelf','Base') where (for the migration) all "Root", 'RootSelf','RootAdmin' groups should be changed to "Base".

In table users:

• get rid of user_id
• rename self_group_id to group_id, place it as first column
• FK this users.group_id to the groups table
• remove users. idUserGodfather which is obsolete (and not used)
• adapt existing services
• adapt other tables where user_id was used, typically replace by user_group_id(11 cases)

Cleaning current data for consistency:

• fix the group type of selfUserGroup's to "UserSelf" (with current prod data, 1 row affected)
• delete all groups with type "UserSelf" but no user having them as self_user_id. (51k rows affected)
• delete all attempts related to non existing users (665 rows affected)
• delete all filters related to non existing users (15 rows)
• set to NULL all groups_groups.inviting_user_id to non-existing users
• put groups_items.creator_user_id nullable and set to NULL all values not matching a user

Our migrations are kind-of tested by applying them on the CI before launching the tests.
Our down-migrations, while they are always written, are not run at all by the CI, so they may be completely erroneous without us noticing it.
It may be very unpleasant to discover that in prod during a critical moment.
So we should include that in the CI testing process.

Fix the behaviour of access right propagation to the following expected behaviour:

For a group A, and let say 2 items M and N; M being the parent of N.
Let's say A is given partial access to M and N (so the 2 groups_items G,M and G,N has both sPartialAccessDate and sCachedPartialAccessDate to "from 2017-01-01".
If G,M is being removed and set to the future, so let's say both sPartialAccessDate and sCachedPartialAccessDate set to "from 2025-01-01"
Then N should not have access to G anymore, i.e. even if G,N still have sPartialAccessDate to "from 2017-01-01", the sCachedPartialAccessDate (the actual access to N) should be "from 2025-01-01".

The computation of sCachedPartialAccessDate for an groups_items should be GREATEST( sCached*AccessDate, MIN(all parents.sCachedPartialAccessDate)) (+ bAccessRestricted rules)

Issue around https://github.com/France-ioi/AlgoreaBackend/blob/master/app/database/group_item_store_compute_all_access.go#L157.

Only for partial access.

Try to categorize properly services in Swagger, try to keep only one tag per service

Add some of the reported missing indices:

• on users_answers.idAttempt to select answers by idAttempt
• on groups.sName for sorting in groupChildrenView
• on groups.iGrade for sorting in groupChildrenView
• on groups.sType for sorting in groupChildrenView