sbp commented on code in PR #309:
URL:
https://github.com/apache/tooling-trusted-releases/pull/309#discussion_r2537033761
##########
atr/config.py:
##########
@@ -86,16 +86,7 @@ class AppConfig:
# FIXME: retrieve the list of admin users from LDAP or oath session /
isRoot
ADMIN_USERS_ADDITIONAL = decouple.config("ADMIN_USERS_ADDITIONAL",
default="", cast=str)
ADMIN_USERS = frozenset(
- {
- "cwells",
- "dfoulks",
- "fluxo",
- "gmcdonald",
- "humbedooh",
- "sbp",
- "tn",
- "wave",
- }
+ {"cwells", "dfoulks", "fluxo", "gmcdonald", "humbedooh", "sbp", "tn",
"wave"}
Review Comment:
Please revert unassociated changes to make the PR easier to review.
##########
atr/get/distribution.py:
##########
@@ -66,55 +68,116 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
## Distributions
block.h2["Distributions"]
for dist in distributions:
- delete_form = await shared.distribution.DeleteForm.create_form(
- data={
- "release_name": dist.release_name,
- "platform": dist.platform.name,
- "owner_namespace": dist.owner_namespace,
- "package": dist.package,
- "version": dist.version,
- }
- )
-
### Platform package version
block.h3(
# Cannot use "#id" here, because the ID contains "."
# If an ID contains ".", htm parses that as a class
id=f"distribution-{dist.identifier}"
)[dist.title]
tbody = htm.tbody[
- shared.distribution.html_tr("Release name", dist.release_name),
- shared.distribution.html_tr("Platform", dist.platform.value.name),
- shared.distribution.html_tr("Owner or Namespace",
dist.owner_namespace or "-"),
- shared.distribution.html_tr("Package", dist.package),
- shared.distribution.html_tr("Version", dist.version),
- shared.distribution.html_tr("Staging", "Yes" if dist.staging else
"No"),
- shared.distribution.html_tr("Upload date", str(dist.upload_date)),
- shared.distribution.html_tr_a("API URL", dist.api_url),
- shared.distribution.html_tr_a("Web URL", dist.web_url),
+ shared.html_tr("Release name", dist.release_name),
+ shared.html_tr("Platform", dist.platform.value.name),
+ shared.html_tr("Owner or Namespace", dist.owner_namespace or "-"),
+ shared.html_tr("Package", dist.package),
+ shared.html_tr("Version", dist.version),
+ shared.html_tr("Staging", "Yes" if dist.staging else "No"),
+ shared.html_tr("Upload date", str(dist.upload_date)),
+ shared.html_tr_a("API URL", dist.api_url),
+ shared.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # Create inline delete form with confirmation dialog (following
projects.py pattern)
Review Comment:
This is currently the correct way to do this, but please add a TODO to the
effect that we should add a `confirm: bool` keyword argument to `form.render`.
This will allow us to use `form.render` and not have to special case forms with
confirmations.
##########
atr/get/distribution.py:
##########
@@ -66,55 +68,116 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
## Distributions
block.h2["Distributions"]
for dist in distributions:
- delete_form = await shared.distribution.DeleteForm.create_form(
- data={
- "release_name": dist.release_name,
- "platform": dist.platform.name,
- "owner_namespace": dist.owner_namespace,
- "package": dist.package,
- "version": dist.version,
- }
- )
-
### Platform package version
block.h3(
# Cannot use "#id" here, because the ID contains "."
# If an ID contains ".", htm parses that as a class
id=f"distribution-{dist.identifier}"
)[dist.title]
tbody = htm.tbody[
- shared.distribution.html_tr("Release name", dist.release_name),
- shared.distribution.html_tr("Platform", dist.platform.value.name),
- shared.distribution.html_tr("Owner or Namespace",
dist.owner_namespace or "-"),
- shared.distribution.html_tr("Package", dist.package),
- shared.distribution.html_tr("Version", dist.version),
- shared.distribution.html_tr("Staging", "Yes" if dist.staging else
"No"),
- shared.distribution.html_tr("Upload date", str(dist.upload_date)),
- shared.distribution.html_tr_a("API URL", dist.api_url),
- shared.distribution.html_tr_a("Web URL", dist.web_url),
+ shared.html_tr("Release name", dist.release_name),
+ shared.html_tr("Platform", dist.platform.value.name),
+ shared.html_tr("Owner or Namespace", dist.owner_namespace or "-"),
+ shared.html_tr("Package", dist.package),
+ shared.html_tr("Version", dist.version),
+ shared.html_tr("Staging", "Yes" if dist.staging else "No"),
+ shared.html_tr("Upload date", str(dist.upload_date)),
+ shared.html_tr_a("API URL", dist.api_url),
+ shared.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # Create inline delete form with confirmation dialog (following
projects.py pattern)
+ delete_form = htm.form(
+ ".d-inline-block.m-0",
+ method="post",
+ action=util.as_url(post.distribution.delete, project=project,
version=version),
+ onsubmit=(
+ f"return confirm('Are you sure you want to delete the
distribution "
+ f"{dist.platform.name} {dist.package} {dist.version}? This
cannot be undone.');"
+ ),
+ )[
+ form.csrf_input(),
+ htpy.input(type="hidden", name="release_name",
value=dist.release_name),
+ htpy.input(type="hidden", name="platform",
value=dist.platform.name),
+ htpy.input(type="hidden", name="owner_namespace",
value=dist.owner_namespace or ""),
+ htpy.input(type="hidden", name="package", value=dist.package),
+ htpy.input(type="hidden", name="version", value=dist.version),
+ htpy.button(".btn.btn-danger.btn-sm", type="submit",
title=f"Delete {dist.title}")[
+ htpy.i(".bi.bi-trash"), " Delete"
+ ],
+ ]
+ block.append(htm.div(".mb-3")[delete_form])
title = f"Distribution list for {project} {version}"
return await template.blank(title, content=block.collect())
+# The delete_get function can now be removed since we're using inline
confirmation
Review Comment:
Let's just remove this.
##########
atr/post/distribution.py:
##########
@@ -49,6 +47,7 @@ async def delete(session: web.Committer, project: str,
version: str) -> web.Werk
package=dd.package,
version=dd.version,
)
+
Review Comment:
Unassociated whitespace change, please remove.
##########
atr/shared/distribution.py:
##########
@@ -155,45 +163,13 @@ def html_tr_a(label: str, value: str | None) ->
htm.Element:
return htm.tr[htm.th[label], htm.td[htm.a(href=value)[value] if value else
"-"]]
-# This function is used for COMPOSE (stage) and FINISH (record)
-# It's also used whenever there is an error
-async def record_form_page(
- fpv: FormProjectVersion, *, extra_content: htm.Element | None = None,
staging: bool = False
+async def record_form_process_page_new(
Review Comment:
We only have functions in `shared` modules when they're used in both `get/`
_and_ `post/` modules. Since this function is only used in
`post/distribution.py`, it must be moved there.
##########
atr/shared/distribution.py:
##########
@@ -155,45 +163,13 @@ def html_tr_a(label: str, value: str | None) ->
htm.Element:
return htm.tr[htm.th[label], htm.td[htm.a(href=value)[value] if value else
"-"]]
-# This function is used for COMPOSE (stage) and FINISH (record)
-# It's also used whenever there is an error
-async def record_form_page(
- fpv: FormProjectVersion, *, extra_content: htm.Element | None = None,
staging: bool = False
+async def record_form_process_page_new(
+ form_data: DistributeForm, project: str, version: str, /, staging: bool =
False
) -> str:
- await release_validated(fpv.project, fpv.version, staging=staging)
-
- # Render the explanation and form
- block = htm.Block()
- html_nav_phase(block, fpv.project, fpv.version, staging)
-
- # Record a manual distribution
- title_and_heading = f"Record a {'staging' if staging else 'manual'}
distribution"
- block.h1[title_and_heading]
- if extra_content:
- block.append(extra_content)
- block.p[
- "Record a distribution of ",
- htm.strong[f"{fpv.project}-{fpv.version}"],
- " using the form below.",
- ]
- block.p[
- "You can also ",
- htm.a(href=util.as_url(get.distribution.list_get, project=fpv.project,
version=fpv.version))[
- "view the distribution list"
- ],
- ".",
- ]
- block.append(forms.render_columns(fpv.form, action=quart.request.path,
descriptions=True))
-
- # Render the page
- return await template.blank(title_and_heading, content=block.collect())
-
-
-async def record_form_process_page(fpv: FormProjectVersion, /, staging: bool =
False) -> str:
- dd = distribution.Data.model_validate(fpv.form.data)
+ dd = distribution.Data.model_validate(form_data.model_dump())
Review Comment:
This form is already being validated by the route functions in
`post/distribution.py` that call this function. It should not be revalidated.
##########
atr/shared/distribution.py:
##########
@@ -270,6 +246,56 @@ async def _alert(message: str) -> str:
return await template.blank("Distribution submitted",
content=block.collect())
+async def record_form_page_new(
+ form_data: DistributeForm,
+ project: str,
+ version: str,
+ *,
+ extra_content: htm.Element | None = None,
+ staging: bool = False,
+) -> str:
+ await release_validated(project, version, staging=staging)
+
+ # Render the explanation and form
+ block = htm.Block()
+ html_nav_phase(block, project, version, staging)
+
+ # Record a manual distribution
+ title_and_heading = f"Record a {'staging' if staging else 'manual'}
distribution"
+ block.h1[title_and_heading]
+ if extra_content:
+ block.append(extra_content)
+ block.p[
+ "Record a distribution of ",
+ htm.strong[f"{project}-{version}"],
+ " using the form below.",
+ ]
+ block.p[
+ "You can also ",
+ htm.a(href=util.as_url(get.distribution.list_get, project=project,
version=version))[
+ "view the distribution list"
+ ],
+ ".",
+ ]
+
+ # Use form.render instead of forms.render_columns
Review Comment:
This comment tells us what's changed, which we don't need to know; we have
revision history for that. Comments should be used to describe things that
aren't obvious about the code but that we need to know.
##########
atr/get/distribution.py:
##########
@@ -66,55 +68,116 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
## Distributions
block.h2["Distributions"]
for dist in distributions:
- delete_form = await shared.distribution.DeleteForm.create_form(
- data={
- "release_name": dist.release_name,
- "platform": dist.platform.name,
- "owner_namespace": dist.owner_namespace,
- "package": dist.package,
- "version": dist.version,
- }
- )
-
### Platform package version
block.h3(
# Cannot use "#id" here, because the ID contains "."
# If an ID contains ".", htm parses that as a class
id=f"distribution-{dist.identifier}"
)[dist.title]
tbody = htm.tbody[
- shared.distribution.html_tr("Release name", dist.release_name),
- shared.distribution.html_tr("Platform", dist.platform.value.name),
- shared.distribution.html_tr("Owner or Namespace",
dist.owner_namespace or "-"),
- shared.distribution.html_tr("Package", dist.package),
- shared.distribution.html_tr("Version", dist.version),
- shared.distribution.html_tr("Staging", "Yes" if dist.staging else
"No"),
- shared.distribution.html_tr("Upload date", str(dist.upload_date)),
- shared.distribution.html_tr_a("API URL", dist.api_url),
- shared.distribution.html_tr_a("Web URL", dist.web_url),
+ shared.html_tr("Release name", dist.release_name),
+ shared.html_tr("Platform", dist.platform.value.name),
+ shared.html_tr("Owner or Namespace", dist.owner_namespace or "-"),
+ shared.html_tr("Package", dist.package),
+ shared.html_tr("Version", dist.version),
+ shared.html_tr("Staging", "Yes" if dist.staging else "No"),
+ shared.html_tr("Upload date", str(dist.upload_date)),
+ shared.html_tr_a("API URL", dist.api_url),
+ shared.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # Create inline delete form with confirmation dialog (following
projects.py pattern)
+ delete_form = htm.form(
+ ".d-inline-block.m-0",
+ method="post",
+ action=util.as_url(post.distribution.delete, project=project,
version=version),
+ onsubmit=(
+ f"return confirm('Are you sure you want to delete the
distribution "
+ f"{dist.platform.name} {dist.package} {dist.version}? This
cannot be undone.');"
+ ),
+ )[
+ form.csrf_input(),
+ htpy.input(type="hidden", name="release_name",
value=dist.release_name),
+ htpy.input(type="hidden", name="platform",
value=dist.platform.name),
+ htpy.input(type="hidden", name="owner_namespace",
value=dist.owner_namespace or ""),
+ htpy.input(type="hidden", name="package", value=dist.package),
+ htpy.input(type="hidden", name="version", value=dist.version),
+ htpy.button(".btn.btn-danger.btn-sm", type="submit",
title=f"Delete {dist.title}")[
+ htpy.i(".bi.bi-trash"), " Delete"
+ ],
+ ]
+ block.append(htm.div(".mb-3")[delete_form])
title = f"Distribution list for {project} {version}"
return await template.blank(title, content=block.collect())
+# The delete_get function can now be removed since we're using inline
confirmation
+# But if you want to keep it for direct URL access, you can redirect to the
list
[email protected]("/distribution/delete/<project>/<version>")
+async def delete_get(session: web.Committer, project: str, version: str) ->
web.WerkzeugResponse:
+ # Redirect to the list page instead of showing a separate confirmation page
+ return await session.redirect(list_get, project=project, version=version)
+
+
@get.committer("/distribution/record/<project>/<version>")
async def record(session: web.Committer, project: str, version: str) -> str:
- form = await
shared.distribution.DistributeForm.create_form(data={"package": project,
"version": version})
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- return await shared.distribution.record_form_page(fpv)
+ await shared.release_validated(project, version, staging=False)
+
+ block = htm.Block()
+ shared.html_nav_phase(block, project, version, staging=False)
+
+ block.h1["Record a manual distribution"]
+ block.p[
+ "Record a distribution of ",
+ htm.strong[f"{project}-{version}"],
+ " using the form below.",
+ ]
+ block.p[
+ "You can also ",
+ htm.a(href=util.as_url(list_get, project=project,
version=version))["view the distribution list"],
+ ".",
+ ]
+
+ # Render the distribution form
+ form_html = form.render(
+ model_cls=shared.DistributeForm,
+ submit_label="Record distribution",
+ action=util.as_url(post.distribution.record_post, project=project,
version=version),
+ defaults={"package": project, "version": version},
+ )
+ block.append(form_html)
+
+ return await template.blank("Record Manual Distribution",
content=block.collect())
@get.committer("/distribution/stage/<project>/<version>")
async def stage(session: web.Committer, project: str, version: str) -> str:
- form = await
shared.distribution.DistributeForm.create_form(data={"package": project,
"version": version})
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- return await shared.distribution.record_form_page(fpv, staging=True)
+ await shared.release_validated(project, version, staging=True)
+
+ block = htm.Block()
+ shared.html_nav_phase(block, project, version, staging=True)
+
+ block.h1["Record a staging distribution"]
+ block.p[
+ "Record a distribution of ",
+ htm.strong[f"{project}-{version}"],
+ " using the form below.",
+ ]
+ block.p[
+ "You can also ",
+ htm.a(href=util.as_url(list_get, project=project,
version=version))["view the distribution list"],
+ ".",
+ ]
+
+ # Render the distribution form
+ form_html = form.render(
+ model_cls=shared.DistributeForm,
+ submit_label="Record distribution",
+ action=util.as_url(post.distribution.stage_post, project=project,
version=version),
+ defaults={"package": project, "version": version},
+ )
+ block.append(form_html)
+
+ return await template.blank("Record Staging Distribution",
content=block.collect())
Review Comment:
We use sentence case for titles, [as mentioned in our code
conventions](https://release-test.apache.org/docs/code-conventions).
##########
atr/shared/distribution.py:
##########
@@ -36,55 +35,64 @@
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
- "Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+class DistributeForm(form.Form):
+ platform: sql.DistributionPlatform = form.label(description="Platform")
+ owner_namespace: str | None = form.label(
Review Comment:
Using `str` allows `""` as a submission, so we typically don't need `|
None`. There is one case where I found it would have been more elegant and type
safe, but arguably typing a value as `None` from the actual HTML is not a
realistic account of what is happening because unlike with checkboxes the field
is not actually omitted. Anyway, let's use `str` for now.
##########
atr/shared/distribution.py:
##########
@@ -36,55 +35,64 @@
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
- "Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+class DistributeForm(form.Form):
+ platform: sql.DistributionPlatform = form.label(description="Platform")
+ owner_namespace: str | None = form.label(
+ description="Owner or Namespace",
+ documentation=(
+ "Who owns or names the package (Maven groupId, npm @scope, Docker
namespace, "
+ "GitHub owner, ArtifactHub repo). Leave blank if not used."
+ ),
+ widget=form.Widget.TEXT,
+ )
+ package: str = form.label("Package", widget=form.Widget.TEXT)
+ version: str = form.label("Version", widget=form.Widget.TEXT)
+ details: bool = form.label(
+ description="Include details",
Review Comment:
We typically don't use `description=` and `documentation=` keywords for the
arguments to `form.label`.
##########
atr/post/distribution.py:
##########
@@ -58,33 +57,14 @@ async def delete(session: web.Committer, project: str,
version: str) -> web.Werk
@post.committer("/distribution/record/<project>/<version>")
-async def record_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv)
- match len(form.errors):
- case 0:
- # Should not happen
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv)
[email protected](shared.DistributeForm)
+async def record_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
+ # Pydantic validation happens automatically in @post.form
Review Comment:
We'd have to add this comment to every single POST handler to be consistent.
Let's just remove this comment instead.
##########
atr/get/distribution.py:
##########
@@ -15,13 +15,15 @@
# specific language governing permissions and limitations
# under the License.
+import htpy
+
import atr.blueprints.get as get
import atr.db as db
-import atr.forms as forms
+import atr.form as form
import atr.htm as htm
import atr.models.sql as sql
import atr.post as post
-import atr.shared as shared
+import atr.shared.distribution as shared
Review Comment:
We don't use this import style. We always import a name `x` as `x`, [as
described in our code
conventions](https://release-test.apache.org/docs/code-conventions). This is
also an unassociated change which should, ideally, be left out of a PR to make
it easier to review.
##########
atr/get/distribution.py:
##########
@@ -66,55 +68,116 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
## Distributions
block.h2["Distributions"]
for dist in distributions:
- delete_form = await shared.distribution.DeleteForm.create_form(
- data={
- "release_name": dist.release_name,
- "platform": dist.platform.name,
- "owner_namespace": dist.owner_namespace,
- "package": dist.package,
- "version": dist.version,
- }
- )
-
### Platform package version
block.h3(
# Cannot use "#id" here, because the ID contains "."
# If an ID contains ".", htm parses that as a class
id=f"distribution-{dist.identifier}"
)[dist.title]
tbody = htm.tbody[
- shared.distribution.html_tr("Release name", dist.release_name),
- shared.distribution.html_tr("Platform", dist.platform.value.name),
- shared.distribution.html_tr("Owner or Namespace",
dist.owner_namespace or "-"),
- shared.distribution.html_tr("Package", dist.package),
- shared.distribution.html_tr("Version", dist.version),
- shared.distribution.html_tr("Staging", "Yes" if dist.staging else
"No"),
- shared.distribution.html_tr("Upload date", str(dist.upload_date)),
- shared.distribution.html_tr_a("API URL", dist.api_url),
- shared.distribution.html_tr_a("Web URL", dist.web_url),
+ shared.html_tr("Release name", dist.release_name),
+ shared.html_tr("Platform", dist.platform.value.name),
+ shared.html_tr("Owner or Namespace", dist.owner_namespace or "-"),
+ shared.html_tr("Package", dist.package),
+ shared.html_tr("Version", dist.version),
+ shared.html_tr("Staging", "Yes" if dist.staging else "No"),
+ shared.html_tr("Upload date", str(dist.upload_date)),
+ shared.html_tr_a("API URL", dist.api_url),
+ shared.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # Create inline delete form with confirmation dialog (following
projects.py pattern)
+ delete_form = htm.form(
+ ".d-inline-block.m-0",
+ method="post",
+ action=util.as_url(post.distribution.delete, project=project,
version=version),
+ onsubmit=(
+ f"return confirm('Are you sure you want to delete the
distribution "
+ f"{dist.platform.name} {dist.package} {dist.version}? This
cannot be undone.');"
+ ),
+ )[
+ form.csrf_input(),
+ htpy.input(type="hidden", name="release_name",
value=dist.release_name),
+ htpy.input(type="hidden", name="platform",
value=dist.platform.name),
+ htpy.input(type="hidden", name="owner_namespace",
value=dist.owner_namespace or ""),
+ htpy.input(type="hidden", name="package", value=dist.package),
+ htpy.input(type="hidden", name="version", value=dist.version),
+ htpy.button(".btn.btn-danger.btn-sm", type="submit",
title=f"Delete {dist.title}")[
+ htpy.i(".bi.bi-trash"), " Delete"
+ ],
+ ]
+ block.append(htm.div(".mb-3")[delete_form])
title = f"Distribution list for {project} {version}"
return await template.blank(title, content=block.collect())
+# The delete_get function can now be removed since we're using inline
confirmation
+# But if you want to keep it for direct URL access, you can redirect to the
list
[email protected]("/distribution/delete/<project>/<version>")
+async def delete_get(session: web.Committer, project: str, version: str) ->
web.WerkzeugResponse:
+ # Redirect to the list page instead of showing a separate confirmation page
+ return await session.redirect(list_get, project=project, version=version)
+
+
@get.committer("/distribution/record/<project>/<version>")
async def record(session: web.Committer, project: str, version: str) -> str:
- form = await
shared.distribution.DistributeForm.create_form(data={"package": project,
"version": version})
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- return await shared.distribution.record_form_page(fpv)
+ await shared.release_validated(project, version, staging=False)
+
+ block = htm.Block()
Review Comment:
All of this code before `form.render` is duplicated in the `stage` function
below, except for the call to `staging` in `html_nav_phase`. We can lift it
into a helper function with `staging` parameterised.
##########
atr/storage/writers/distributions.py:
##########
@@ -161,8 +164,12 @@ async def record_from_data(
match api_oc:
case outcome.Result(result):
pass
- case outcome.Error():
- raise storage.AccessError("Failed to get API response from
distribution platform")
+ case outcome.Error(error):
+ # Log the actual error for debugging
+ import atr.log as log
Review Comment:
This should be top level unless it causes an import cycle.
##########
atr/post/distribution.py:
##########
@@ -58,33 +57,14 @@ async def delete(session: web.Committer, project: str,
version: str) -> web.Werk
@post.committer("/distribution/record/<project>/<version>")
-async def record_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv)
- match len(form.errors):
- case 0:
- # Should not happen
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv)
[email protected](shared.DistributeForm)
+async def record_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
+ # Pydantic validation happens automatically in @post.form
+ return await shared.record_form_process_page_new(form, project, version,
staging=False)
@post.committer("/distribution/stage/<project>/<version>")
-async def stage_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv,
staging=True)
- match len(form.errors):
- case 0:
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv, staging=True)
[email protected](shared.DistributeForm)
+async def stage_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
Review Comment:
We don't call forms `form` because that conflicts with the `atr.form` module
name when imported in our standard style. For `ExampleForm` we use the name
`example_form`, so for `DistributeForm` that would be `distribute_form`.
##########
atr/post/distribution.py:
##########
@@ -58,33 +57,14 @@ async def delete(session: web.Committer, project: str,
version: str) -> web.Werk
@post.committer("/distribution/record/<project>/<version>")
-async def record_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv)
- match len(form.errors):
- case 0:
- # Should not happen
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv)
[email protected](shared.DistributeForm)
+async def record_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
Review Comment:
The `form` variable here should be called `distribute_form`, and the
function should be called `record_selected`; see the `stage_post` review for
details.
##########
atr/shared/distribution.py:
##########
@@ -155,45 +163,13 @@ def html_tr_a(label: str, value: str | None) ->
htm.Element:
return htm.tr[htm.th[label], htm.td[htm.a(href=value)[value] if value else
"-"]]
-# This function is used for COMPOSE (stage) and FINISH (record)
-# It's also used whenever there is an error
-async def record_form_page(
- fpv: FormProjectVersion, *, extra_content: htm.Element | None = None,
staging: bool = False
+async def record_form_process_page_new(
+ form_data: DistributeForm, project: str, version: str, /, staging: bool =
False
Review Comment:
We reserve the name `form_data` for form data that we get from Quart or
ASFQuart. `DistributeForm` should always have the name `distribute_form`.
##########
atr/post/distribution.py:
##########
@@ -58,33 +57,14 @@ async def delete(session: web.Committer, project: str,
version: str) -> web.Werk
@post.committer("/distribution/record/<project>/<version>")
-async def record_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv)
- match len(form.errors):
- case 0:
- # Should not happen
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv)
[email protected](shared.DistributeForm)
+async def record_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
+ # Pydantic validation happens automatically in @post.form
+ return await shared.record_form_process_page_new(form, project, version,
staging=False)
@post.committer("/distribution/stage/<project>/<version>")
-async def stage_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv,
staging=True)
- match len(form.errors):
- case 0:
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv, staging=True)
[email protected](shared.DistributeForm)
+async def stage_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
Review Comment:
We try to avoid using a module's own name in any of its functions, unless
there is no obvious alternative. In this case, the function should be called
`stage_selected`, because we use `selected` when `project` and `version`
parameters are present in the URL.
##########
atr/storage/writers/distributions.py:
##########
@@ -23,6 +23,7 @@
import aiohttp
import sqlalchemy.exc as exc
+from sqlalchemy import text
Review Comment:
We don't use this import style, [according to our code
conventions](https://release-test.apache.org/docs/code-conventions).
##########
atr/shared/distribution.py:
##########
@@ -202,7 +178,7 @@ async def _alert(message: str) -> str:
div = htm.Block(htm.div(".alert.alert-danger"))
div.p[message]
collected = div.collect()
- return await record_form_page(fpv, extra_content=collected,
staging=staging)
+ return await record_form_page_new(form_data, project, version,
extra_content=collected, staging=staging)
Review Comment:
Instead of calling a whole function separate from the GET route to render
the page again, we should set a flash message and redirect. Then this function
could disappear; it probably only exists due to code which has since been
refactored out anyway as it's only called once.
##########
atr/form.py:
##########
@@ -561,14 +561,26 @@ def _get_choices(field_info: pydantic.fields.FieldInfo)
-> list[tuple[str, str]]
inner_type = args[0]
if isinstance(inner_type, type) and issubclass(inner_type,
enum.Enum):
# This is an enum type wrapped in Annotated, from Enum[T] or
Set[T]
- return [(member.value, member.value) for member in inner_type]
+ return [
+ (
+ member.name,
+ member.value.name if hasattr(member.value, "name")
else str(member.value),
+ )
+ for member in inner_type
+ ]
if origin is set:
args = get_args(annotation)
if args:
enum_class = args[0]
if isinstance(enum_class, type) and issubclass(enum_class,
enum.Enum):
- return [(member.value, member.value) for member in enum_class]
+ return [
Review Comment:
This won't be necessary after using a wrapper enum; see the "Check for
single enum types" comment below for details.
##########
atr/form.py:
##########
@@ -577,7 +589,13 @@ def _get_choices(field_info: pydantic.fields.FieldInfo) ->
list[tuple[str, str]]
# Check for plain enum types, e.g. when Pydantic unwraps form.Enum[T]
if isinstance(annotation, type) and issubclass(annotation, enum.Enum):
- return [(member.value, member.value) for member in annotation]
+ return [
Review Comment:
This won't be necessary after using a wrapper enum; see the "Check for
single enum types" comment below for details.
##########
atr/get/distribution.py:
##########
@@ -66,55 +68,116 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
## Distributions
block.h2["Distributions"]
for dist in distributions:
- delete_form = await shared.distribution.DeleteForm.create_form(
- data={
- "release_name": dist.release_name,
- "platform": dist.platform.name,
- "owner_namespace": dist.owner_namespace,
- "package": dist.package,
- "version": dist.version,
- }
- )
-
### Platform package version
block.h3(
# Cannot use "#id" here, because the ID contains "."
# If an ID contains ".", htm parses that as a class
id=f"distribution-{dist.identifier}"
)[dist.title]
tbody = htm.tbody[
- shared.distribution.html_tr("Release name", dist.release_name),
- shared.distribution.html_tr("Platform", dist.platform.value.name),
- shared.distribution.html_tr("Owner or Namespace",
dist.owner_namespace or "-"),
- shared.distribution.html_tr("Package", dist.package),
- shared.distribution.html_tr("Version", dist.version),
- shared.distribution.html_tr("Staging", "Yes" if dist.staging else
"No"),
- shared.distribution.html_tr("Upload date", str(dist.upload_date)),
- shared.distribution.html_tr_a("API URL", dist.api_url),
- shared.distribution.html_tr_a("Web URL", dist.web_url),
+ shared.html_tr("Release name", dist.release_name),
+ shared.html_tr("Platform", dist.platform.value.name),
+ shared.html_tr("Owner or Namespace", dist.owner_namespace or "-"),
+ shared.html_tr("Package", dist.package),
+ shared.html_tr("Version", dist.version),
+ shared.html_tr("Staging", "Yes" if dist.staging else "No"),
+ shared.html_tr("Upload date", str(dist.upload_date)),
+ shared.html_tr_a("API URL", dist.api_url),
+ shared.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # Create inline delete form with confirmation dialog (following
projects.py pattern)
+ delete_form = htm.form(
+ ".d-inline-block.m-0",
+ method="post",
+ action=util.as_url(post.distribution.delete, project=project,
version=version),
+ onsubmit=(
+ f"return confirm('Are you sure you want to delete the
distribution "
+ f"{dist.platform.name} {dist.package} {dist.version}? This
cannot be undone.');"
+ ),
+ )[
+ form.csrf_input(),
+ htpy.input(type="hidden", name="release_name",
value=dist.release_name),
+ htpy.input(type="hidden", name="platform",
value=dist.platform.name),
+ htpy.input(type="hidden", name="owner_namespace",
value=dist.owner_namespace or ""),
+ htpy.input(type="hidden", name="package", value=dist.package),
+ htpy.input(type="hidden", name="version", value=dist.version),
+ htpy.button(".btn.btn-danger.btn-sm", type="submit",
title=f"Delete {dist.title}")[
+ htpy.i(".bi.bi-trash"), " Delete"
+ ],
+ ]
+ block.append(htm.div(".mb-3")[delete_form])
title = f"Distribution list for {project} {version}"
return await template.blank(title, content=block.collect())
+# The delete_get function can now be removed since we're using inline
confirmation
+# But if you want to keep it for direct URL access, you can redirect to the
list
[email protected]("/distribution/delete/<project>/<version>")
+async def delete_get(session: web.Committer, project: str, version: str) ->
web.WerkzeugResponse:
+ # Redirect to the list page instead of showing a separate confirmation page
+ return await session.redirect(list_get, project=project, version=version)
+
+
@get.committer("/distribution/record/<project>/<version>")
async def record(session: web.Committer, project: str, version: str) -> str:
- form = await
shared.distribution.DistributeForm.create_form(data={"package": project,
"version": version})
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- return await shared.distribution.record_form_page(fpv)
+ await shared.release_validated(project, version, staging=False)
+
+ block = htm.Block()
+ shared.html_nav_phase(block, project, version, staging=False)
+
+ block.h1["Record a manual distribution"]
+ block.p[
+ "Record a distribution of ",
+ htm.strong[f"{project}-{version}"],
+ " using the form below.",
+ ]
+ block.p[
+ "You can also ",
+ htm.a(href=util.as_url(list_get, project=project,
version=version))["view the distribution list"],
+ ".",
+ ]
+
+ # Render the distribution form
+ form_html = form.render(
+ model_cls=shared.DistributeForm,
+ submit_label="Record distribution",
+ action=util.as_url(post.distribution.record_post, project=project,
version=version),
+ defaults={"package": project, "version": version},
+ )
+ block.append(form_html)
+
+ return await template.blank("Record Manual Distribution",
content=block.collect())
Review Comment:
We use sentence case for titles, [as mentioned in our code
conventions](https://release-test.apache.org/docs/code-conventions).
##########
atr/shared/distribution.py:
##########
@@ -36,55 +35,64 @@
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
- "Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+class DistributeForm(form.Form):
+ platform: sql.DistributionPlatform = form.label(description="Platform")
+ owner_namespace: str | None = form.label(
+ description="Owner or Namespace",
+ documentation=(
+ "Who owns or names the package (Maven groupId, npm @scope, Docker
namespace, "
+ "GitHub owner, ArtifactHub repo). Leave blank if not used."
+ ),
+ widget=form.Widget.TEXT,
+ )
+ package: str = form.label("Package", widget=form.Widget.TEXT)
+ version: str = form.label("Version", widget=form.Widget.TEXT)
+ details: bool = form.label(
+ description="Include details",
+ documentation="Include the details of the distribution in the
response",
+ widget=form.Widget.CHECKBOX,
Review Comment:
This can be dropped, like with `TEXT`.
##########
atr/form.py:
##########
@@ -640,6 +658,10 @@ def _get_widget_type(field_info:
pydantic.fields.FieldInfo) -> Widget: # noqa:
if annotation in (int, float):
return Widget.NUMBER
+ # Check for single enum types
Review Comment:
We already have `form.Enum` for this which annotates a single enum type, but
`sql.DistributionPlatform` isn't compatible because it's structured. Rather
than adding special code for it, we should probably take the same approach used
in `shared/ignore.py` and use a wrapper enum. Then we could use
`form.Enum[NameOfTheWrapperEnum]` as the type of the form field.
##########
atr/form.py:
##########
@@ -561,14 +561,26 @@ def _get_choices(field_info: pydantic.fields.FieldInfo)
-> list[tuple[str, str]]
inner_type = args[0]
if isinstance(inner_type, type) and issubclass(inner_type,
enum.Enum):
# This is an enum type wrapped in Annotated, from Enum[T] or
Set[T]
- return [(member.value, member.value) for member in inner_type]
+ return [
Review Comment:
This won't be necessary after using a wrapper enum; see the "Check for
single enum types" comment below for details.
##########
atr/shared/distribution.py:
##########
@@ -36,55 +35,64 @@
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
- "Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+class DistributeForm(form.Form):
+ platform: sql.DistributionPlatform = form.label(description="Platform")
+ owner_namespace: str | None = form.label(
+ description="Owner or Namespace",
+ documentation=(
+ "Who owns or names the package (Maven groupId, npm @scope, Docker
namespace, "
+ "GitHub owner, ArtifactHub repo). Leave blank if not used."
+ ),
+ widget=form.Widget.TEXT,
Review Comment:
We don't need to use `Widget.TEXT`. We only need to set the widget in
general when we want to use something other than the default choice, which is
this case is already `TEXT`. Same for other fields.
##########
atr/form.py:
##########
@@ -640,6 +658,10 @@ def _get_widget_type(field_info:
pydantic.fields.FieldInfo) -> Widget: # noqa:
if annotation in (int, float):
return Widget.NUMBER
+ # Check for single enum types
+ if hasattr(annotation, "__members__"):
Review Comment:
We don't need this because we already have `form.Enum` as noted above, but
if we were to do this then we would check for it being a subclass of
`enum.Enum` instead.
##########
atr/shared/distribution.py:
##########
@@ -36,55 +35,64 @@
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
- "Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+class DistributeForm(form.Form):
+ platform: sql.DistributionPlatform = form.label(description="Platform")
+ owner_namespace: str | None = form.label(
+ description="Owner or Namespace",
+ documentation=(
+ "Who owns or names the package (Maven groupId, npm @scope, Docker
namespace, "
+ "GitHub owner, ArtifactHub repo). Leave blank if not used."
+ ),
+ widget=form.Widget.TEXT,
+ )
+ package: str = form.label("Package", widget=form.Widget.TEXT)
+ version: str = form.label("Version", widget=form.Widget.TEXT)
+ details: bool = form.label(
+ description="Include details",
+ documentation="Include the details of the distribution in the
response",
+ widget=form.Widget.CHECKBOX,
)
- package = forms.string("Package", placeholder="E.g. artifactId or
package-name")
- version = forms.string("Version", placeholder="E.g. 1.2.3, without a
leading v")
- details = forms.checkbox("Include details", description="Include the
details of the distribution in the response")
- submit = forms.submit("Record distribution")
-
- async def validate(self, extra_validators: dict | None = None) -> bool:
- if not await super().validate(extra_validators):
- return False
- if not self.platform.data:
- return False
- default_owner_namespace =
self.platform.data.value.default_owner_namespace
- requires_owner_namespace =
self.platform.data.value.requires_owner_namespace
- owner_namespace = self.owner_namespace.data
- # TODO: We should disable the owner_namespace field if it's not
required
- # But that would be a lot of complexity
- # And this validation, which we need to keep, is complex enough
- if default_owner_namespace and (not owner_namespace):
- self.owner_namespace.data = default_owner_namespace
- if requires_owner_namespace and (not owner_namespace):
- msg = f'Platform "{self.platform.data.name}" requires an owner or
namespace.'
- return forms.error(self.owner_namespace, msg)
- if (not requires_owner_namespace) and (not default_owner_namespace)
and owner_namespace:
- msg = f'Platform "{self.platform.data.name}" does not require an
owner or namespace.'
- return forms.error(self.owner_namespace, msg)
- return True
-
-
[email protected]
-class FormProjectVersion:
- form: DistributeForm
- project: str
- version: str
+
+ @pydantic.field_validator("platform", mode="before")
+ @classmethod
+ def validate_platform(cls, value: Any) -> sql.DistributionPlatform:
+ if isinstance(value, sql.DistributionPlatform):
+ return value
+ if isinstance(value, str):
Review Comment:
We'll be changing this anyway when we use the wrapper enum in`form.Enum`,
but presumably this value is either one or the other. We shouldn't need to
check for both.
##########
atr/shared/distribution.py:
##########
@@ -36,55 +35,64 @@
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
- "Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+class DistributeForm(form.Form):
+ platform: sql.DistributionPlatform = form.label(description="Platform")
+ owner_namespace: str | None = form.label(
+ description="Owner or Namespace",
+ documentation=(
+ "Who owns or names the package (Maven groupId, npm @scope, Docker
namespace, "
+ "GitHub owner, ArtifactHub repo). Leave blank if not used."
+ ),
+ widget=form.Widget.TEXT,
+ )
+ package: str = form.label("Package", widget=form.Widget.TEXT)
+ version: str = form.label("Version", widget=form.Widget.TEXT)
+ details: bool = form.label(
+ description="Include details",
+ documentation="Include the details of the distribution in the
response",
+ widget=form.Widget.CHECKBOX,
)
- package = forms.string("Package", placeholder="E.g. artifactId or
package-name")
- version = forms.string("Version", placeholder="E.g. 1.2.3, without a
leading v")
- details = forms.checkbox("Include details", description="Include the
details of the distribution in the response")
- submit = forms.submit("Record distribution")
-
- async def validate(self, extra_validators: dict | None = None) -> bool:
- if not await super().validate(extra_validators):
- return False
- if not self.platform.data:
- return False
- default_owner_namespace =
self.platform.data.value.default_owner_namespace
- requires_owner_namespace =
self.platform.data.value.requires_owner_namespace
- owner_namespace = self.owner_namespace.data
- # TODO: We should disable the owner_namespace field if it's not
required
- # But that would be a lot of complexity
- # And this validation, which we need to keep, is complex enough
- if default_owner_namespace and (not owner_namespace):
- self.owner_namespace.data = default_owner_namespace
- if requires_owner_namespace and (not owner_namespace):
- msg = f'Platform "{self.platform.data.name}" requires an owner or
namespace.'
- return forms.error(self.owner_namespace, msg)
- if (not requires_owner_namespace) and (not default_owner_namespace)
and owner_namespace:
- msg = f'Platform "{self.platform.data.name}" does not require an
owner or namespace.'
- return forms.error(self.owner_namespace, msg)
- return True
-
-
[email protected]
-class FormProjectVersion:
- form: DistributeForm
- project: str
- version: str
+
+ @pydantic.field_validator("platform", mode="before")
+ @classmethod
+ def validate_platform(cls, value: Any) -> sql.DistributionPlatform:
+ if isinstance(value, sql.DistributionPlatform):
+ return value
+ if isinstance(value, str):
+ try:
+ return sql.DistributionPlatform[value]
+ except KeyError:
+ raise ValueError(f"Invalid platform: {value}")
+ raise ValueError(f"Platform must be a string or DistributionPlatform,
got {type(value)}")
+
+ @pydantic.model_validator(mode="after")
+ def validate_owner_namespace(self):
+ if not self.platform:
+ raise ValueError("Platform is required")
+
+ default_owner_namespace = self.platform.value.default_owner_namespace
+ requires_owner_namespace = self.platform.value.requires_owner_namespace
+
+ # Set default if needed and not provided
+ if default_owner_namespace and not self.owner_namespace:
+ self.owner_namespace = default_owner_namespace
+
+ # Validate requirements
+ if requires_owner_namespace and not self.owner_namespace:
+ raise ValueError(f'Platform "{self.platform.name}" requires an
owner or namespace.')
+
+ if not requires_owner_namespace and not default_owner_namespace and
self.owner_namespace:
Review Comment:
Please ensure that all subexpressions are parenthesized, as pointed out in
our [code conventions](https://release-test.apache.org/docs/code-conventions).
Same goes for prior lines in this method, and potentially elsewhere.
##########
atr/post/distribution.py:
##########
@@ -17,28 +17,26 @@
from __future__ import annotations
-import quart
-
import atr.blueprints.post as post
import atr.db as db
import atr.get as get
import atr.models.distribution as distribution
-import atr.shared as shared
+import atr.shared.distribution as shared
import atr.storage as storage
import atr.web as web
@post.committer("/distribution/delete/<project>/<version>")
-async def delete(session: web.Committer, project: str, version: str) ->
web.WerkzeugResponse:
- form = await shared.distribution.DeleteForm.create_form(data=await
quart.request.form)
- dd = distribution.DeleteData.model_validate(form.data)
[email protected](shared.DeleteForm)
+async def delete(session: web.Committer, form: shared.DeleteForm, project:
str, version: str) -> web.WerkzeugResponse:
+ dd = distribution.DeleteData.model_validate(form.model_dump())
Review Comment:
This form is already being validated by the route function decorator. It
should not be revalidated.
##########
atr/post/distribution.py:
##########
@@ -58,33 +57,14 @@ async def delete(session: web.Committer, project: str,
version: str) -> web.Werk
@post.committer("/distribution/record/<project>/<version>")
-async def record_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv)
- match len(form.errors):
- case 0:
- # Should not happen
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv)
[email protected](shared.DistributeForm)
+async def record_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
+ # Pydantic validation happens automatically in @post.form
+ return await shared.record_form_process_page_new(form, project, version,
staging=False)
@post.committer("/distribution/stage/<project>/<version>")
-async def stage_post(session: web.Committer, project: str, version: str) ->
str:
- form = await shared.distribution.DistributeForm.create_form(data=await
quart.request.form)
- fpv = shared.distribution.FormProjectVersion(form=form, project=project,
version=version)
- if await form.validate():
- return await shared.distribution.record_form_process_page(fpv,
staging=True)
- match len(form.errors):
- case 0:
- await quart.flash("Ambiguous submission errors",
category="warning")
- case 1:
- await quart.flash("There was 1 submission error", category="error")
- case _ as n:
- await quart.flash(f"There were {n} submission errors",
category="error")
- return await shared.distribution.record_form_page(fpv, staging=True)
[email protected](shared.DistributeForm)
+async def stage_post(session: web.Committer, form: shared.DistributeForm,
project: str, version: str) -> str:
+ # Pydantic validation happens automatically in @post.form
Review Comment:
Let's remove this comment.
##########
atr/post/distribution.py:
##########
@@ -17,28 +17,26 @@
from __future__ import annotations
-import quart
-
import atr.blueprints.post as post
import atr.db as db
import atr.get as get
import atr.models.distribution as distribution
-import atr.shared as shared
+import atr.shared.distribution as shared
Review Comment:
As mentioned on `get/distribution.py`, we don't use this import style and
this is an unassociated change.
##########
atr/storage/writers/distributions.py:
##########
@@ -120,6 +121,8 @@ async def record(
self.__data.add(distribution)
try:
await self.__data.commit()
+ # Force WAL checkpoint to ensure data is immediately visible to
other sessions
Review Comment:
[Checkpointing does not make data visible to other
sessions](https://sqlite.org/wal.html#concurrency):
> A checkpoint can run concurrently with readers, however the checkpoint
must stop when it reaches a page in the WAL that is past the end mark of any
current reader.
The operation that [makes data available to _subsequent_
transactions](https://sqlite.org/isolation.html) is `commit`, which we are
already doing. Once data is in the WAL, [it is available to
readers](https://sqlite.org/wal.html#concurrency):
> When a reader needs a page of content, it first checks the WAL to see if
that page appears there, and if so it pulls in the last copy of the page that
occurs in the WAL prior to the reader's end mark.
It is [not possible in sqlite to make new data available to _existing_
transactions](https://sqlite.org/lang_transaction.html#read_transactions_versus_write_transactions)
unless [a mode of an obsolete feature is
enabled](https://sqlite.org/isolation.html):
> If two database connections share the same cache and the reader has
enabled the [read_uncommitted
pragma](https://sqlite.org/pragma.html#pragma_read_uncommitted), then the
reader will be able to see changes made by the writer before the writer
transaction commits. The combined use of [shared cache
mode](https://sqlite.org/sharedcache.html) and the [read_uncommitted
pragma](https://sqlite.org/pragma.html#pragma_read_uncommitted) is the only way
that one database connection can see uncommitted changes on a different
database connection.
In addition to being marked as obsolete, the documentation says that
"[shared-cache mode is discouraged](https://sqlite.org/sharedcache.html)";
`read_uncommitted` only applies when shared cache mode is enabled. We try
instead to ensure that transactions are open for as short a duration as
possible, which has other benefits such as not blocking checkpointing.
Checkpointing has no effect even on database connections with a shared cache
and `read_uncommitted` set because the uncommitted data is read from the cache,
not from the WAL.
##########
atr/shared/distribution.py:
##########
@@ -36,55 +35,64 @@
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
- "Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+class DistributeForm(form.Form):
+ platform: sql.DistributionPlatform = form.label(description="Platform")
+ owner_namespace: str | None = form.label(
+ description="Owner or Namespace",
+ documentation=(
+ "Who owns or names the package (Maven groupId, npm @scope, Docker
namespace, "
+ "GitHub owner, ArtifactHub repo). Leave blank if not used."
+ ),
+ widget=form.Widget.TEXT,
+ )
+ package: str = form.label("Package", widget=form.Widget.TEXT)
+ version: str = form.label("Version", widget=form.Widget.TEXT)
+ details: bool = form.label(
Review Comment:
Using `bool` makes this field required. The correct type is `Bool`. We need
to make it clearer which `form` types are mandatory and which are optional.
They're almost all optional but I didn't want to add `Opt` to them all, so I
think it would be better to have a prefix (or suffix) for the few mandatory
ones only.
##########
atr/post/distribution.py:
##########
@@ -17,28 +17,26 @@
from __future__ import annotations
-import quart
-
import atr.blueprints.post as post
import atr.db as db
import atr.get as get
import atr.models.distribution as distribution
-import atr.shared as shared
+import atr.shared.distribution as shared
import atr.storage as storage
import atr.web as web
@post.committer("/distribution/delete/<project>/<version>")
-async def delete(session: web.Committer, project: str, version: str) ->
web.WerkzeugResponse:
- form = await shared.distribution.DeleteForm.create_form(data=await
quart.request.form)
- dd = distribution.DeleteData.model_validate(form.data)
[email protected](shared.DeleteForm)
+async def delete(session: web.Committer, form: shared.DeleteForm, project:
str, version: str) -> web.WerkzeugResponse:
Review Comment:
The form should be called `delete_form`, otherwise this overrides the
`atr.form` module name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
