500 Server Error When Commenting #65

New Issue

Victor Chedeville · 2024-03-31T21:04:27+02:00

Victor Chedeville commented

2024-03-31 21:04:27 +02:00

I get a 500 Server Error when commenting on an addon in the approval queue

Windows 11
Chromium web browser (Vivaldi)

I get a `500 Server Error` when commenting on an addon in the approval queue Windows 11 Chromium web browser (Vivaldi)

Pablo Vazquez commented

2024-04-02 16:33:44 +02:00

Can confirm when starting the message with a space:

Can confirm when starting the message with a space: <video src="/attachments/b7adbf73-c821-4bbe-943f-22da427402e4" title="Screen Recording 2024-04-02 at 16.31.58.mov" controls></video>

Screen Recording 2024-04-02 at 16.31.58.mov

2.1 MiB

Pablo Vazquez added the

Type

Bug

label 2024-04-02 16:33:57 +02:00

Oleg-Komarov commented

2024-04-04 10:24:43 +02:00

A bit of a side question:

I've noticed that the comment form is also displayed for users who are not maintainers of a given extension nor moderators. Is it intentional?

If we want other people to leave comments on the queue, what would be the use case?

==

500 error happens because we don't have a way to render the form submission errors:

these "passing by" (not maintainers, not moderators) users mentioned above trigger an error because they don't have the type select field, so the submitted form is lacking a mandatory field
a blank message (only spaces) also triggers an error

There are two steps to fix:

make sure that client-side validation catches all the common cases, so that we never have to render a server-side error page: we need to block form submission is the message is blank, hide the form completely from "passers-by" or pass a hidden type="CMT" for them if we want them to comment.
render a server-side form error page. it would make sense to render it in the full context of the original /approval-queue/<slug>/ where the form is submitted.

It's best to combine both for a smooth UX and for handling anything unexpected, but we could start with just the second one, especially if we don't want to allow the passer-by comments.

A bit of a side question: I've noticed that the comment form is also displayed for users who are not maintainers of a given extension nor moderators. Is it intentional? If we want other people to leave comments on the queue, what would be the use case? == 500 error happens because we don't have a way to render the form submission errors: 1. these "passing by" (not maintainers, not moderators) users mentioned above trigger an error because they don't have the `type` select field, so the submitted form is lacking a mandatory field 2. a blank message (only spaces) also triggers an error There are two steps to fix: 1. make sure that client-side validation catches all the common cases, so that we never have to render a server-side error page: we need to block form submission is the message is blank, hide the form completely from "passers-by" or pass a hidden `type="CMT"` for them if we want them to comment. 2. render a server-side form error page. it would make sense to render it in the full context of the original `/approval-queue/<slug>/` where the form is submitted. It's best to combine both for a smooth UX and for handling anything unexpected, but we could start with just the second one, especially if we don't want to allow the passer-by comments.

Pablo Vazquez commented

2024-04-04 16:44:02 +02:00

I've noticed that the comment form is also displayed for users who are not maintainers of a given extension nor moderators. Is it intentional?

Yes. Maybe @dfelinto can confirm, but I believe permissions go as follows:

Leave a Comment: Anyone. Every user is welcome to help the review process by commenting.
Mark as Awaiting Review: Only extension maintainers and moderators.
Mark as Awaiting Changes: Only extension maintainers and moderators. Used to signal that the review is on hold.
Mark as Approved: Only moderators.

> I've noticed that the comment form is also displayed for users who are not maintainers of a given extension nor moderators. Is it intentional? Yes. Maybe @dfelinto can confirm, but I believe permissions go as follows: * Leave a **Comment**: Anyone. Every user is welcome to help the review process by commenting. * Mark as **Awaiting Review**: Only extension maintainers and moderators. * Mark as **Awaiting Changes**: Only extension maintainers and moderators. Used to signal that the review is on hold. * Mark as **Approved**: Only moderators.

👍 1

Dalai Felinto commented

2024-04-04 18:40:50 +02:00

I've noticed that the comment form is also displayed for users who are not maintainers of a given extension nor moderators. Is it intentional?

Definitively intentional. Everyone is welcome to help with moderation (by commenting). This is also how we will eventually spot new moderators to join the team.

In short, what Pablo listed here is correct.

> I've noticed that the comment form is also displayed for users who are not maintainers of a given extension nor moderators. Is it intentional? Definitively intentional. Everyone is welcome to help with moderation (by commenting). This is also how we will eventually spot new moderators to join the team. ---------- In short, what Pablo listed here is correct.

👍 1

Dalai Felinto referenced this issue

2024-04-08 11:50:51 +02:00

Extensions Website - Backend Road to Beta #70

Anna Sirota referenced this issue from a commit

2024-04-19 17:28:45 +02:00

Fix 500 when commenting as non-staff/non-moderator

Anna Sirota closed this issue

2024-04-19 17:28:45 +02:00

Oleg-Komarov referenced this issue from a commit

2024-04-30 20:00:54 +02:00

Fix regression of #65 (500 error when commenting)