Set up forms app with authentication #1497

jchristgit · 2025-03-27T18:32:44Z

This begins the work of integrating the forms backend into the site.
Changes, until complete, will be merged into the forms tracking
branch.

Copilot

Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.

Files not reviewed (1)

.coveragerc: Language not supported

Comments suppressed due to low confidence (1)

pydis_site/apps/forms/tests/test_api.py:127

[nitpick] Consider revising or removing the informal comment to maintain a professional tone in test files.

# the ultimate power trip...

pydis_site/settings.py

pydis_site/apps/forms/permissions.py

pydis_site/apps/forms/discord.py

jchristgit · 2025-04-13T18:41:33Z

The next person who sends "co""pilot" A"I" onto my review will have me send a Bengalese Tiger to their doorstep. And it will be very, very hungry.

This begins the work of integrating the forms backend into the site. Changes, until complete, will be merged into the `forms` tracking branch.

jb3 · 2025-07-20T18:52:26Z

pydis_site/apps/forms/discord.py

https://discord.com/developers/docs/reference#user-agent

We should probably add our user agent in here.

jb3 · 2025-07-20T19:00:00Z

pydis_site/apps/forms/migrations/0001_initial_updated.py

We don't need:

Nearly as much role information as we are currently storing. We use this almost solely for calculating scopes and can probably get away with name and ID (if even that, most of this data is validated on the fly)

We don't need member information, just user information and whether they are in the guild or not.

As a sample of a filled response object:

{ "_id" : "7d880da8-0f06-4f4e-bc1d-9987eefa0d51", "user" : { "username" : "...", "id" : "...", "discriminator" : "...", "avatar" : "...", "bot" : null, "system" : null, "locale" : "en-US", "verified" : null, "email" : null, "flags" : 64, "premium_type" : 2, "public_flags" : 64, "admin" : false }, "antispam" : { "ip_hash" : "...", "user_agent_hash" : "...", "captcha_pass" : false }, "response" : { "header" : false, "question" : "...", "attribution" : "Yes" }, "form_id" : "qa", "timestamp" : "..." }

We don't currently store users and I don't know if we should start storing users, sounds like a bit of a headache for data protection if we have another store of user data that is separate from form responses.

jb3 · 2025-07-20T19:00:34Z

pydis_site/apps/forms/migrations/0001_initial_updated.py

+                    models.BigIntegerField(
+                        help_text="Total permissions of the member in the channel.",
+                        null=True,
+                        verbose_name="Pending screening",


Copy-paste typo.

jb3 · 2025-07-20T19:02:13Z

pydis_site/apps/forms/models.py

+    )
+
+
+# XXX: This duplicates the role object from the API app. The role object in the


We can definitely get away with the role information in the API app.

MarkKoz · 2025-07-20T20:28:26Z

pydis_site/apps/forms/admin.py

@@ -0,0 +1,2 @@
+
+# Register your models here.


Can we get rid of this file if it's empty anyway?

MarkKoz · 2025-07-20T20:29:27Z

pydis_site/apps/forms/apps.py

+class FormsConfig(AppConfig):
+    """Django AppConfig for the forms app."""
+
+    name = "pydis_site.apps.forms"


Is this supposed to match the module name? Can we avoid hardcoding the name? Maybe something with importlib can help?

MarkKoz · 2025-07-20T20:32:43Z

pydis_site/apps/forms/discord.py

+
+def fetch_member_details(member_id: int) -> models.DiscordMember | None:
+    """Get a member by ID from the configured guild using the discord API."""
+    with httpx.Client() as client:


Is it expensive to create a new client for each request?

MarkKoz · 2025-07-20T20:33:06Z

pydis_site/apps/forms/discord.py

+
+
+def fetch_member_details(member_id: int) -> models.DiscordMember | None:
+    """Get a member by ID from the configured guild using the discord API."""


Suggested change

"""Get a member by ID from the configured guild using the discord API."""

"""Get a member by ID from the configured guild using the Discord API."""

MarkKoz · 2025-07-20T20:36:25Z

pydis_site/apps/forms/discord.py

+
+def get_roles(*, force_refresh: bool = False, stale_after: int = 60 * 60 * 24) -> tuple[models.DiscordRole, ...]:
+    """
+    Get a tuple of all roles from the cache, or discord API if not available.


Suggested change

Get a tuple of all roles from the cache, or discord API if not available.

Get a tuple of all roles from the cache, or Discord API if not available.

MarkKoz · 2025-07-20T20:38:30Z

pydis_site/apps/forms/discord.py

+
+    ## Keyword arguments
+
+    - `force_refresh` (`bool`): Skip the cache and always update the roles from


I think putting the type in the docs is redundant when we're using type annotations.

MarkKoz · 2025-07-20T20:39:01Z

pydis_site/apps/forms/discord.py

+    - `stale_after` (`int`): Seconds after which to consider the stored roles
+      as stale and to refresh them.


This argument does not exist for this function.

MarkKoz · 2025-07-20T20:42:12Z

pydis_site/apps/forms/authentication.py

+    @property
+    def display_name(self) -> str:
+        """Return username and discriminator as display name."""
+        return f"{self.payload['username']}#{self.payload['discriminator']}"


Didn't Discord do away with discriminators?

MarkKoz · 2025-07-20T20:44:48Z

pydis_site/apps/forms/authentication.py

+
+    def is_admin(self) -> bool:
+        """Return whether this user is an administrator."""
+        self.admin = models.Admin.objects.filter(id=self.payload["id"]).exists()


Should self.admin be "private"? Currently, if someone inspects self.admin directly without calling is_admin(), its value may not be correct. The requirement to call is_admin() first is not obvious.

MarkKoz · 2025-07-20T20:49:38Z

pydis_site/apps/forms/authentication.py

+        user = FormsUser(
+            token,
+            user_details,
+            discord.get_member(user_details["id"]),


Do we need error handling for this call or is it fine to let the exception bubble up?

jchristgit force-pushed the forms-bootstrap branch 2 times, most recently from 0a51298 to 8b6dbeb Compare March 27, 2025 19:08

jchristgit force-pushed the forms-bootstrap branch from 8b6dbeb to 603e046 Compare April 4, 2025 17:16

jb3 requested a review from Copilot April 6, 2025 21:13

Copilot AI reviewed Apr 6, 2025

View reviewed changes

pydis_site/settings.py Show resolved Hide resolved

pydis_site/apps/forms/permissions.py Show resolved Hide resolved

pydis_site/apps/forms/discord.py Show resolved Hide resolved

jchristgit force-pushed the forms-bootstrap branch from 603e046 to 6525554 Compare July 19, 2025 19:24

jchristgit requested a review from MarkKoz as a code owner July 19, 2025 19:24

python-discord-policy-bot bot requested a review from a team July 19, 2025 19:24

jchristgit requested review from ChrisLovering and jb3 July 19, 2025 19:25

Set up forms app with authentication

0c19201

This begins the work of integrating the forms backend into the site. Changes, until complete, will be merged into the `forms` tracking branch.

jchristgit force-pushed the forms-bootstrap branch from 6525554 to 0c19201 Compare July 19, 2025 19:35

jb3 requested changes Jul 20, 2025

View reviewed changes

MarkKoz reviewed Jul 20, 2025

View reviewed changes

		)


		# XXX: This duplicates the role object from the API app. The role object in the



		def fetch_member_details(member_id: int) -> models.DiscordMember \| None:
		"""Get a member by ID from the configured guild using the discord API."""

	Get a tuple of all roles from the cache, or discord API if not available.
	Get a tuple of all roles from the cache, or Discord API if not available.


		## Keyword arguments

		- `force_refresh` (`bool`): Skip the cache and always update the roles from

		- `stale_after` (`int`): Seconds after which to consider the stored roles
		as stale and to refresh them.

Uh oh!

Set up forms app with authentication #1497

Are you sure you want to change the base?

Set up forms app with authentication #1497

Conversation

jchristgit commented Mar 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jchristgit commented Apr 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!