Skip to content

Conversation

@kjw142857
Copy link
Contributor

@kjw142857 kjw142857 commented Apr 5, 2023

Aim is to remove unique score checking for contest entries in backend in preparation for our frontend pull request to implement our contest voting tierlist, which accepts non-unique scores as submissions.

Am changing these 5 files:

assessments.ex - removed reference to non-unique submission votes causing an error and changed corresponding error message.
submission_votes_test.exs - removed test for unique votes.
submission_votes.ex - commented out unique score checker (unique_constraint)
Added new migration file 20230331010500_remove_unique_score_constraint.exs - reversible migration that removes the unique score index from the database, such that it can accept non-unique score entries.
answer_controller_test.exs - removed test case that checked for duplicate score, changed test case to check for updated error message in assessments.ex

@coveralls
Copy link

coveralls commented Apr 5, 2023

Coverage Status

Coverage: 95.608%. Remained the same when pulling f993440 on kjw142857:master into 9bf6a5c on source-academy:master.

Copy link
Contributor

@YaleChen299 YaleChen299 left a comment

Choose a reason for hiding this comment

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

LTGM! Thank you for the good work! Just some minor nits for comments.

|> case do
{:ok, _result} -> {:ok, nil}
{:error, _name, _changset, _error} -> {:error, :vote_not_unique}
# error type has been changed to :invalid_vote
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this comment since we have reviewed this PR.


{:error, :vote_not_unique} ->
{:error, {:bad_request, "Invalid vote or vote is not unique! Vote is not saved."}}
# Description changed from :vote_not_unique and corresponding message changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is Okay to remove the comment, since other developer might be buffled by the word "change", since they might not be following this project. This change history is captured in github so it is okay.

@required_fields ~w(voter_id submission_id question_id)a
@optional_fields ~w(score)a

# |> unique_constraint(:unique, name: :unique_score) (Unique constraint removed)
Copy link
Contributor

Choose a reason for hiding this comment

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

For here, change the comment to a factual statement like "there is no unique constraint...blah blah".

{:error, changeset} = Repo.insert(second_entry)
refute changeset.valid?
end
# Removed test for unique submission vote
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly here

@YaleChen299 YaleChen299 merged commit 6837435 into source-academy:master Apr 9, 2023
@kjw142857 kjw142857 added Enhancement New feature or request _backend The issue concerns backend component labels Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

_backend The issue concerns backend component Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants