Skip to content

Add mentions field to Reminders model#370

Merged
Den4200 merged 12 commits into
masterfrom
role-reminders
Jul 20, 2020
Merged

Add mentions field to Reminders model#370
Den4200 merged 12 commits into
masterfrom
role-reminders

Conversation

@kosayoda
Copy link
Copy Markdown
Contributor

@kosayoda kosayoda commented Jul 16, 2020

To facilitate bot/#1026, this PR adds a field to store the IDs of Member and Role objects.

No validation is done other than ensuring the ID is greater than 0, since we just ignore the ID if it doesn't convert to a valid Member/Role object on the bot side.

@kosayoda kosayoda added area: backend Related to internal functionality and utilities area: API Related to or causes API changes labels Jul 16, 2020
@kosayoda kosayoda requested a review from a team as a code owner July 16, 2020 07:27
@kosayoda kosayoda requested review from SebastiaanZ and scragly July 16, 2020 07:27
@ghost ghost added the needs 2 approvals label Jul 16, 2020
@kosayoda kosayoda mentioned this pull request Jul 16, 2020
Merged
@MarkKoz MarkKoz added language: python Involves Python code priority: 2 - normal Normal Priority type: feature New feature or request labels Jul 16, 2020
MarkKoz
MarkKoz requested changes Jul 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

Is there any compelling reason we'd still need to track the author? Can we just lump their ID in with the rest of the mentions?

Comment thread pydis_site/apps/api/tests/test_reminders.py Outdated
Comment thread pydis_site/apps/api/viewsets/bot/reminder.py Outdated
@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Jul 19, 2020
@kosayoda
Copy link
Copy Markdown
Contributor Author

kosayoda commented Jul 19, 2020

Is there any compelling reason we'd still need to track the author? Can we just lump their ID in with the rest of the mentions?

Currently, the author field is a FK to a User. Getting all the reminders set by a particular User is trivial, and although we could filter by an index to the array, I don't think it's ideal.

More importantly, since I'm using an ArrayField to store the list of IDs, there's also no way to cascade delete reminders when the author leaves the server (assuming we go through with storing the author ID in the mentions field).

I'll address your other changes.

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Jul 19, 2020

Ah right we need to know the author cause we have a list reminders command.

kosayoda added 2 commits July 19, 2020 12:49
@kosayoda
Use literal integers for mentions ID in test
f6111a3 
Since the mentions field stores static IDs and not foreign keys, there
is no need to create the objects for the test.
@kosayoda
Fix misleading documentation
387b763
@ghost ghost added needs 2 approvals and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jul 19, 2020
Den4200
Den4200 approved these changes Jul 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Den4200 Den4200 left a comment

Choose a reason for hiding this comment

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

It looks good to me.

@Den4200 Den4200 merged commit d720d56 into master Jul 20, 2020
@Den4200 Den4200 deleted the role-reminders branch July 20, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarkKoz MarkKoz MarkKoz approved these changes

@Den4200 Den4200 Den4200 approved these changes

@scragly scragly Awaiting requested review from scragly

@SebastiaanZ SebastiaanZ Awaiting requested review from SebastiaanZ

Assignees

No one assigned

Labels

area: API Related to or causes API changes area: backend Related to internal functionality and utilities language: python Involves Python code priority: 2 - normal Normal Priority type: feature New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kosayoda @MarkKoz @Den4200