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

Quality: Drop legacy dissolve reason columns, remove manual migration #1187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

realmayus
Copy link
Contributor

Back in #853, we migrated the integer-based dissolveReason to dissolveReasonEnum.
Then, dissolveReasonEnum became obsolete when we allowed the user to select multiple dissolveReasons in #941.

If the following query can be trusted,

SELECT "id", "studentId", "pupilId", "dissolveReasons", "dissolveReasonEnum", "dissolvedBy"
FROM match
WHERE dissolved = true
  AND NOT (
    array_length("dissolveReasons", 1) = 1
    AND "dissolveReasons"[1] = "dissolveReasonEnum"
  );

we only have five matches where dissolveReasons != [dissolveReasonEnum]:

  id   | studentId | pupilId | dissolveReasons | dissolveReasonEnum | dissolvedBy 
-------+-----------+---------+-----------------+--------------------+-------------
 17184 |     17613 |   28983 | {noMoreTime}    | accountDeactivated | pupil
 17350 |     17809 |   18845 | {ghosted}       | accountDeactivated | pupil
 17334 |     17801 |   29619 | {noMoreTime}    | other              | student
 17085 |     17550 |   29304 | {noMoreTime}    | accountDeactivated | pupil
 17227 |     17751 |   17654 | {noMoreTime}    | other              | pupil
(5 rows)

While I'm not 100% sure (due to logs being deleted regularly), I believe these are reactivated matches?
We ended up not writing to dissolveReasonEnum anymore; we're just updating dissolveReasons nowadays.

I think we should be fine with dropping dissolveReason and dissolveReasonEnum, but I would appreciate another set of eyes on this.

@realmayus realmayus temporarily deployed to backend-fix-remove-lega-sbiwal January 20, 2025 15:20 Inactive
@realmayus realmayus changed the title Fix: Drop legacy dissolve reason columns, remove manual migration Quality: Drop legacy dissolve reason columns, remove manual migration Jan 20, 2025
@Jonasdoubleyou
Copy link
Member

I am a bit surprised that your query returns no match with more than one dissolve reason?

@realmayus
Copy link
Contributor Author

realmayus commented Jan 22, 2025

I am a bit surprised that your query returns no match with more than one dissolve reason?

@Jonasdoubleyou We currently only have the option to select multiple reasons in the backend, the frontend match dissolve dialog has not been adjusted yet

SELECT "id" from match where array_length("dissolveReasons", 1) > 1; returns 0 rows

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