offtopicnames active attribute and support for PUT and PATCH request #508

RohanJnr · 2021-05-19T07:49:33Z

closes #490
linked to python-discord/bot#1568

Changes

New attribute active for OffTopicName.
Can query based on the active attribute of OffTopicName.
Patch and Put support for OffTopicName.
DETAIL view support for OffTopicName.
Tests modified as required.

…channel-names

…NameListSerializer.

coveralls · 2021-06-09T21:19:39Z

Coverage remained the same at 100.0% when pulling c4e2eda on RohanJnr:otn_softdel into b52587d on python-discord:main.

jchristgit

This looks great! A small comment. Nice use of Walrus.

pydis_site/apps/api/serializers.py

jchristgit · 2021-07-22T18:18:32Z

@RohanJnr thanks for the docstring update.

There's currently an error in CI due to conflicting migrations:

CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0070_auto_20210519_0545, 0071_increase_message_content_4000 in api).
20
To fix them run 'python manage.py makemigrations --merge'

Could you look into fixing this?

It also seems the branch needs to be updated.

RohanJnr · 2021-07-22T18:39:35Z

Looks like I forgot to push the new commits, haha. Will do when I am at my pc tmw 👍

jchristgit

I like this. I have a few questions, but apart from that, I like this.

pydis_site/apps/api/serializers.py

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

RohanJnr · 2021-11-04T18:44:10Z

Thanks for the review again @jchristgit :D, I will work on them asap.

jchristgit

Solid work. Thank you!

One minor suggestion for the future: Instead of using the autogenerated names by Django for migrations (like auto_20210519_0545), it's nice to have a short name describing the migration.

D0rs4n · 2021-11-27T18:28:47Z

Hey! Would you mind opening an issue (and a PR if you'd like to) in the API repository regarding the database model changes? @RohanJnr

Bluenix2 · 2021-12-11T18:51:42Z

@RohanJnr thanks for the docstring update.

There's currently an error in CI due to conflicting migrations:
CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0070_auto_20210519_0545, 0071_increase_message_content_4000 in api).
20
To fix them run 'python manage.py makemigrations --merge'
Could you look into fixing this?

It also seems the branch needs to be updated.

The fix for this was the 0072_merge migration I see correct?

Do want to give 0070_auto_20210519_0545 a more meaningful name?

jchristgit · 2021-12-12T13:02:01Z

The fix for this was the 0072_merge migration I see correct?

Yes

Do want to give 0070_auto_20210519_0545 a more meaningful name?

I think due to the dependencies of the other migrations on this migration this will get a bit hairy, unfortunately :(

Bluenix2 · 2021-12-25T00:47:03Z

The behaviour of the PATCH/PUT method is a little unexpected. You can send a request to /api/bot/off-topic-channel-names/existing-name with the body {"name": "another-one"}... which causes another off-topic name to be created. Should this be changed or should I approve? Considering that is my only complaint..

That said when using PATCH correctly it worked nicely and made the names no longer show up when specifying ?active=true (as expected from the changes by this PR).

RohanJnr · 2021-12-25T19:59:14Z

Practically, The PATCH method allows modification of all fields on the model. We could leave it as it is as we will only be patching the active field. If we really want to restrict the PATCH to 1 field only, i.e, the active field, we can add a check to make sure the request body only contains the active field. But, I am not sure if we should have this check or leave it as it is?

Bluenix2 · 2021-12-25T20:12:45Z

Practically, The PATCH method allows modification of all fields on the model. We could leave it as it is as we will only be patching the active field. If we really want to restrict the PATCH to 1 field only, i.e, the active field, we can add a check to make sure the request body only contains the active field. But, I am not sure if we should have this check or leave it as it is?

What is unexpected is the fact that you can create a new off-topic-name by PUTting (this is primarily what I tried, don't remember if it works the same with PATCH) to an existing off-topic-name. I don't really understand what even caused the support for these new methods as it is done implicitly by Django so I am not sure how much it would take to change this behaviour.

RohanJnr · 2021-12-25T20:49:35Z

My comment above applies to PUT also. The reason for the creation of a new object is django behaviour towards primary keys. This is from the docs: `The primary key field is read-only. If you change the value of the primary key on an existing object and then save it, a new object will be created alongside the old one.` https://docs.djangoproject.com/en/4.0/ref/models/fields/#primary-key

Bluenix2

I think what we discussed is fine then if it seems like it would be a lot of code to change it - it's not a big deal. Thanks for this, let's get it merged!

jchristgit · 2024-06-19T19:01:58Z

Sorry, wrong project...

RohanJnr and others added 12 commits May 19, 2021 13:13

Add active field to OffTopicChannelName model.

d061ab1

Serialize name and active attribute.

8232b11

Use ModelViewSet to add support for PUT and PATCH request.

b2819a4

Return a list of ot-names only for GET request only to bot/off-topic-…

28de14b

…channel-names

Return a list of ot-names only for GET request only to bot/off-topic-…

69f4dc1

…channel-names

Query off topic names based on Active attribute.

87e25f7

Lint file and remove update() method declaration from OffTopicChannel…

da1056f

…NameListSerializer.

Fix bug: Do not force active param.

f0534b8

Add tests for active params.

832880c

fix conflicts?.

023b171

Use assertListEqual where applicable instead of assertEqual.

3795b6d

Merge branch 'main' into otn_softdel

c444b82

RohanJnr marked this pull request as ready for review June 9, 2021 21:03

RohanJnr requested review from Akarys42 and Den4200 as code owners June 9, 2021 21:03

Use sets to compare 2 un-ordered lists.

8fdabfb

RohanJnr force-pushed the otn_softdel branch from 12f10b2 to 8fdabfb Compare June 9, 2021 21:18

jchristgit approved these changes Jul 18, 2021

View reviewed changes

pydis_site/apps/api/serializers.py Outdated Show resolved Hide resolved

Update docstring.

e7b4da7

RohanJnr and others added 3 commits July 24, 2021 19:23

Merge branch 'main' into otn_softdel

3317854

fix CommandError: Conflicting migrations detected.

d52f21c

Merge branch 'main' into otn_softdel

d106260

Xithrius requested review from jchristgit and jb3 September 25, 2021 06:47

jchristgit requested changes Oct 5, 2021

View reviewed changes

Pull from upstream/main and resolve conflicts.

747a7b9

Use framework defination of method.

6e1d38d

RohanJnr requested a review from jchristgit November 5, 2021 05:29

Remove get_queryset() and add new class variable .

4377c6d

Xithrius added area: API Related to or causes API changes area: backend Related to internal functionality and utilities priority: 2 - normal Normal Priority type: enhancement Changes or improvements to existing features labels Nov 12, 2021

jchristgit approved these changes Nov 16, 2021

View reviewed changes

jchristgit enabled auto-merge November 16, 2021 18:31

jchristgit requested a review from HassanAbouelela December 3, 2021 10:22

jchristgit requested review from Bluenix2 and removed request for jb3, Akarys42 and Den4200 December 11, 2021 12:52

RohanJnr and others added 2 commits December 13, 2021 11:14

Merge branch 'main' into otn_softdel

4693de0

Merge migrations to avoid conflicts.

1261efd

RohanJnr force-pushed the otn_softdel branch from f3964c3 to 1261efd Compare December 13, 2021 05:57

Bluenix2 approved these changes Dec 25, 2021

View reviewed changes

Merge branch 'main' into otn_softdel

c4e2eda

jchristgit merged commit 4ab7972 into python-discord:main Dec 28, 2021

RohanJnr mentioned this pull request Jan 23, 2022

Return random off topic names which are Active only #644

Merged

This comment was marked as outdated.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

offtopicnames active attribute and support for PUT and PATCH request #508

offtopicnames active attribute and support for PUT and PATCH request #508

RohanJnr commented May 19, 2021 •

edited

Loading

coveralls commented Jun 9, 2021 •

edited

Loading

jchristgit left a comment

jchristgit commented Jul 22, 2021

RohanJnr commented Jul 22, 2021

jchristgit left a comment

RohanJnr commented Nov 4, 2021

jchristgit left a comment

D0rs4n commented Nov 27, 2021 •

edited

Loading

Bluenix2 commented Dec 11, 2021

jchristgit commented Dec 12, 2021

Bluenix2 commented Dec 25, 2021 •

edited

Loading

RohanJnr commented Dec 25, 2021

Bluenix2 commented Dec 25, 2021

RohanJnr commented Dec 25, 2021 via email •

edited

Loading

Bluenix2 left a comment

This comment was marked as outdated.

jchristgit commented Jun 19, 2024

offtopicnames active attribute and support for PUT and PATCH request #508

offtopicnames active attribute and support for PUT and PATCH request #508

Conversation

RohanJnr commented May 19, 2021 • edited Loading

Changes

coveralls commented Jun 9, 2021 • edited Loading

jchristgit left a comment

Choose a reason for hiding this comment

jchristgit commented Jul 22, 2021

RohanJnr commented Jul 22, 2021

jchristgit left a comment

Choose a reason for hiding this comment

RohanJnr commented Nov 4, 2021

jchristgit left a comment

Choose a reason for hiding this comment

D0rs4n commented Nov 27, 2021 • edited Loading

Bluenix2 commented Dec 11, 2021

jchristgit commented Dec 12, 2021

Bluenix2 commented Dec 25, 2021 • edited Loading

RohanJnr commented Dec 25, 2021

Bluenix2 commented Dec 25, 2021

RohanJnr commented Dec 25, 2021 via email • edited Loading

Bluenix2 left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

jchristgit commented Jun 19, 2024

RohanJnr commented May 19, 2021 •

edited

Loading

coveralls commented Jun 9, 2021 •

edited

Loading

D0rs4n commented Nov 27, 2021 •

edited

Loading

Bluenix2 commented Dec 25, 2021 •

edited

Loading

RohanJnr commented Dec 25, 2021 via email •

edited

Loading