Rework nominations #447

ks129 · 2021-02-22T09:40:31Z

Note! This HAVE to be merged EXACTLY the same time as bot PR

Changes

Instead of one nominations table, there are now 2 tables: Nominations and nomination entries. The nominations table got one new field reviewed (boolean). Here are schemas of these tables:

Table: nomination

active (boolean)
user (User)
inserted_at (datetime)
end_reason (string)
ended_at (datetime)
reviewed (boolean)

Table: nomination entries

nomination (Nomination)
actor (User)
reason (string)
inserted_at (datetime)

Now is also allowed multiple nominations per-user and they are created as nomination entries. One user can create only 1 entry for each nomination.

Also updated tests to match with all these changes.

What to test?

Is the migration script properly moving data from old schema to new.
Is GET routes working as expected?
Is the POST route creating a new nomination entry if nomination already exists?
Is the PATCH route reviewed field setting works?
Is the PATCH patching behaviors correct?
Is tests covering everything (there is one place where I had to ignore it)?

Closes #445

…able system

…e actor

coveralls · 2021-02-22T09:42:10Z

Coverage remained the same at 100.0% when pulling 3429fe5 on ks123/nomination/split into f17075f on master.

SebastiaanZ

I ran out of time for now, but I'm a few commits in. I've just gone over the code as of now and I wanted to give my feedback early, rather than late, as I'm not sure when I'll have time again.

Things I haven't reviewed yet:

Grammar
Functional test

As a general comment: None of the commit messages I've read have a meaningful commit message body and some of the titles are a bit too long. Ideally, a commit message should explain the what and why of the change you're committing to the history instead of having just a single summary line.

Some of these commits are fairly sizeable and you've made a number of choices within those commits; try to add the reasoning behind those choices into the commit message body so that reasoning becomes part of the git history of the repository.

These are few links @lemonsaurus typically shares when talking about commit messages:

https://chris.beams.io/posts/git-commit/
https://thoughtbot.com/blog/5-useful-tips-for-a-better-commit-message
http://ablogaboutcode.com/2011/03/23/proper-git-commit-messages-and-an-elegant-git-history
https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

A more extreme example: https://dhwthompson.com/2019/my-favourite-git-commit

This page is also really nice: https://mtlynch.io/code-review-love/

pydis_site/apps/api/models/bot/nomination.py

pydis_site/apps/api/serializers.py

pydis_site/apps/api/viewsets/bot/nomination.py

In order to use entries in serializer without manually setting entries key we have to use related_name option to automatically fetch all related entries.

Set it here so we don't have to set it every place where we fetch entries.

This doesn't make sense to have 3 small migrations for one PR, so I merged 2 existing migrations and 1 new, ordering and related_name adding migrations to one.

After setting related_name in NominationEntry model nomination field, we can just provide serializer and DRF automatically fetch all related entries.

After moving entries to nomination serializer we can get rid from GET request handlers and let DRF handle this. Also PATCH and POST handlers got some simplification by removing manual entries setting.

Entries isn't handled manually anymore so these tests have no point.

For string fields NULL as default is not suggested, so use empty string instead.

…ry serializer

HassanAbouelela · 2021-03-01T08:06:17Z

I've looked over the new schema, and from a functional standpoint, it seems good.

Here are the old vs new schemas if anyone wants to compare (you can play around with it here):

Old
New

lemonsaurus

I'm mostly okay with this PR. There are grammar issues, but it looks like it does what it needs to do.

pydis_site/apps/api/viewsets/bot/nomination.py

jb3

Mainly grammar + code style fixes, but otherwise looks good.

pydis_site/apps/api/viewsets/bot/nomination.py

Co-authored-by: Leon Sandøy <leon.haland@gmail.com> Co-authored-by: Joe Banks <joseph@josephbanks.me>

Co-authored-by: Joe Banks <joseph@josephbanks.me>

pydis_site/apps/api/models/bot/nomination.py

mbaruh

A few more

pydis_site/apps/api/models/bot/nomination.py

pydis_site/apps/api/viewsets/bot/nomination.py

ks129 added 13 commits February 22, 2021 08:21

Split nomination model to 2 tables and create migrations

9ac4773

Change nominations admin interface and add nomination entries interface

ff46d9f

Change nominations serializer and add nomination entry serializer

4bc55a5

Update nominations viewset GET and POST to make this working with 2-t…

51c57d3

…able system

Add reviewed field to nomination serializer

71388ff

Migrate PATCH request for 2-table nominations system

e17da1f

Disable creating multiple nomination entries of one nomination for on…

0ae69d9

…e actor

Add rollback to nominations table split migration

2fb5bf5

Wrap validation errors to []

698cbc1

Import NominationEntry to models __init__.py

6591270

Change NominationEntry import location in serializers

126b508

Small improvements in nomination viewset

3907122

Update nomination-related tests to cover recent table-splitting changes

9af0579

ks129 added area: API Related to or causes API changes priority: 1 - high High Priority labels Feb 22, 2021

ks129 requested review from Akarys42 and Den4200 as code owners February 22, 2021 09:40

SebastiaanZ requested changes Feb 23, 2021

View reviewed changes

ks129 added 8 commits February 24, 2021 08:27

Set related_name option of nomination field of NominationEntry

f731e4d

In order to use entries in serializer without manually setting entries key we have to use related_name option to automatically fetch all related entries.

Set default ordering of NominationEntry to inserted_at decreasing

1fc5470

Set it here so we don't have to set it every place where we fetch entries.

Merge nomination migrations

bb88dc6

This doesn't make sense to have 3 small migrations for one PR, so I merged 2 existing migrations and 1 new, ordering and related_name adding migrations to one.

Add entries field to Nomination serializer

7682b56

After setting related_name in NominationEntry model nomination field, we can just provide serializer and DRF automatically fetch all related entries.

Simplify nominations viewset

3edb416

After moving entries to nomination serializer we can get rid from GET request handlers and let DRF handle this. Also PATCH and POST handlers got some simplification by removing manual entries setting.

Remove GET nomination test for entries

774e7c0

Entries isn't handled manually anymore so these tests have no point.

Make default value of nomination entry reason to empty string

cd03f74

For string fields NULL as default is not suggested, so use empty string instead.

Add comment about manually defining nomination field of NominationEnt…

98c60cf

…ry serializer

ks129 requested a review from SebastiaanZ February 24, 2021 06:48

ks129 mentioned this pull request Mar 3, 2021

Migrate talentpool to new API schema python-discord/bot#1445

Merged

lemonsaurus approved these changes Mar 4, 2021

View reviewed changes

jb3 approved these changes Mar 4, 2021

View reviewed changes

SebastiaanZ approved these changes Mar 4, 2021

View reviewed changes

pydis_site/apps/api/viewsets/bot/nomination.py Outdated Show resolved Hide resolved

pydis_site/apps/api/viewsets/bot/nomination.py Outdated Show resolved Hide resolved

pydis_site/apps/api/viewsets/bot/nomination.py Outdated Show resolved Hide resolved

ks129 and others added 4 commits March 5, 2021 07:48

Fix grammar of nomination endpoints documentation

54eca6d

Co-authored-by: Leon Sandøy <leon.haland@gmail.com> Co-authored-by: Joe Banks <joseph@josephbanks.me>

Replace double quotes with single quotes

72693a6

Co-authored-by: Joe Banks <joseph@josephbanks.me>

Use double quotes instead apostrophe because string contain "don't"

9075231

Fix grammar of error messages and change tests to match with changes

d8751ad

mbaruh requested changes Mar 6, 2021

View reviewed changes

pydis_site/apps/api/models/bot/nomination.py Outdated Show resolved Hide resolved

mbaruh requested changes Mar 6, 2021

View reviewed changes

ks129 added 4 commits March 6, 2021 14:09

Fix grammar of nomination models

53a9f52

Change as -> to in nomination viewset docs

ce5a082

Fix grammar of nomination viewset command about single entry for user

9202d92

Fix grammar of unknown actor error and change tests

48eff5d

ks129 requested a review from mbaruh March 6, 2021 12:14

mbaruh approved these changes Mar 6, 2021

View reviewed changes

Merge branch 'master' into ks123/nomination/split

3429fe5

mbaruh merged commit e0fc61a into master Mar 8, 2021

mbaruh deleted the ks123/nomination/split branch March 8, 2021 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework nominations #447

Rework nominations #447

ks129 commented Feb 22, 2021

coveralls commented Feb 22, 2021 •

edited

Loading

SebastiaanZ left a comment

HassanAbouelela commented Mar 1, 2021

lemonsaurus left a comment

jb3 left a comment

mbaruh left a comment

Rework nominations #447

Rework nominations #447

Conversation

ks129 commented Feb 22, 2021

Changes

What to test?

coveralls commented Feb 22, 2021 • edited Loading

SebastiaanZ left a comment

Choose a reason for hiding this comment

HassanAbouelela commented Mar 1, 2021

lemonsaurus left a comment

Choose a reason for hiding this comment

jb3 left a comment

Choose a reason for hiding this comment

mbaruh left a comment

Choose a reason for hiding this comment

coveralls commented Feb 22, 2021 •

edited

Loading