chore: docs updated - #391
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe pull request restructures Goodmap documentation, updates Sphinx configuration, adds installation and operational guides, expands configuration and API references, and records known JSON validation and request-size limitations. ChangesDocumentation and maintenance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/development.rst`:
- Around line 48-50: Update the e2e dependency installation instruction in the
development documentation to preserve the repository root as the working
directory, using the existing command-targeting pattern rather than changing
directories; ensure subsequent root-level make commands run from the repository
root.
In `@docs/index.rst`:
- Around line 17-19: Update the “Running a Goodmap” text in the documentation to
clarify that Python programming experience is not required, while preserving
that Python 3.10 or newer is needed to run the application.
In `@docs/installation.rst`:
- Around line 50-54: Update the API version response example following the curl
command to use a clearly generic placeholder instead of the release-specific
“2.0.0a5” value, while preserving the documented JSON structure.
- Around line 33-37: Update the Poetry installation example in the project map
setup section to explicitly enable pre-release selection for goodmap, using the
supported command-line option or an equivalent dependency constraint with
allow-prereleases enabled.
In `@goodmap/admin_api.py`:
- Around line 187-194: Add a before-request authorization guard for the admin
API blueprint near the existing endpoint definitions, reusing the application’s
established session/admin role mechanism to reject anonymous and non-admin
callers with 401 or 403 before any route handler runs. Preserve access for
authorized admins and add coverage for both anonymous and authenticated
non-admin requests.
In `@goodmap/goodmap.py`:
- Around line 191-196: Replace the ineffective MAX_CONTENT_LENGTH guard in the
application configuration with a finite global request-body limit large enough
for the 5 MB multipart upload and protocol overhead, while preserving explicitly
configured limits. Apply stricter limits to JSON endpoints where needed, and add
coverage for default, configured, and oversized requests.
In `@goodmap/templates/goodmap-admin.html`:
- Around line 39-49: Resolve the schema mismatch in the Locations tab by driving
its columns, filters, add/edit form, and rendering logic from the location
schema passed by goodmap.py::admin, including safe handling when accessible_by
is absent; alternatively, disable the tab when required fields are missing and
display a clear message. Ensure the /api/category/accessible_by request and
renderLocations() do not assume bridges fields, and add an integration test
covering a schema without accessible_by.
In `@Makefile`:
- Around line 40-46: Fix the guaranteed-failing verify-json-data workflow by
implementing the missing goodmap.data_validator module, or remove the target and
update docs/data-source.rst plus all CI callers accordingly. If implementing it,
build the model via data_models.location.create_location_model using the file’s
map section, validate every point, and exit non-zero on the first invalid input.
Add coverage for both valid and invalid JSON data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aabcb0f1-41c1-48d4-9e2c-df7f2f68518c
📒 Files selected for processing (11)
Makefiledocs/api.rstdocs/conf.pydocs/development.rstdocs/index.rstdocs/installation.rstdocs/plugins.rstdocs/quickstart.rstgoodmap/admin_api.pygoodmap/goodmap.pygoodmap/templates/goodmap-admin.html
💤 Files with no reviewable changes (1)
- docs/api.rst
| make -C frontend install # frontend work | ||
| cd e2e-tests && poetry install # e2e work | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/development.rst outline/lines =="
wc -l docs/development.rst
sed -n '1,180p' docs/development.rst
echo
echo "== repository top-level relevant files =="
git ls-files | awk -F/ '{print $1}' | sort -u | sed -n '1,80p'
fd -a 'Makefile|poetry\.lock|pyproject\.toml' . | sed 's#^\./##' | sort
echo
echo "== commands and directory changes in docs/development.rst =="
python3 - <<'PY'
from pathlib import Path
p = Path('docs/development.rst')
for i, line in enumerate(p.read_text().splitlines(), start=1):
if 'cd ' in line or 'Makefile' in line or 'make ' in line or 'poetry' in line:
print(f"{i}: {line}")
PYRepository: Problematy/goodmap
Length of output: 7942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('docs/development.rst').read_text()
lines = text.splitlines()
for i, line in enumerate(lines, start=1):
if 'cd ' in line and 'e2e-tests' in line:
print(f"line {i}: {line}")
# Simulate current directory changes in the setup block.
cwd = Path('/repo-root')
for j in range(i, min(i, len(lines))):
pass
if 'cd e2e-tests && poetry install' in line:
if 'cd ' in line and '::=' in line or False:
pass
cwd = cwd / 'e2e-tests'
print("cwd_after_setup=", cwd)
continue
if 'make ' in line and not line.strip().startswith('..'):
print(f"make line {i}: {line}")
# Simple make invocations documented in the rest of the file resolve from repo root.
PY
printf '\n== Makefile presence for later commands ==\n'
# No need to run repository code; just report tracked Makefiles needed by the documented commands.
for f in Makefile frontend/Makefile e2e-tests/Makefile docs/Makefile; do
[ -f "$f" ] && echo "$f exists" || echo "$f missing"
doneRepository: Problematy/goodmap
Length of output: 2172
Keep the repository root as the working directory.
Line [49] changes into e2e-tests, but later commands run make without -C, so readers can invoke the wrong Makefile or fail in the same shell.
Keep the root directory
- cd e2e-tests && poetry install # e2e work
+ (cd e2e-tests && poetry install) # e2e work; keep the root directory📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| make -C frontend install # frontend work | |
| cd e2e-tests && poetry install # e2e work | |
| make -C frontend install # frontend work | |
| (cd e2e-tests && poetry install) # e2e work; keep the root directory | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/development.rst` around lines 48 - 50, Update the e2e dependency
installation instruction in the development documentation to preserve the
repository root as the working directory, using the existing command-targeting
pattern rather than changing directories; ensure subsequent root-level make
commands run from the repository root.
| **Running a Goodmap** | ||
| You want an instance of your own. Install it, write ``config.yml``, author the data, | ||
| moderate what users submit, put it in production. No Python required. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that Python programming is not required.
docs/installation.rst requires Python 3.10 or newer. The text “No Python required” can incorrectly imply that the runtime is optional.
Proposed wording
- put it in production. No Python required.
+ put it in production. No Python programming required.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Running a Goodmap** | |
| You want an instance of your own. Install it, write ``config.yml``, author the data, | |
| moderate what users submit, put it in production. No Python required. | |
| **Running a Goodmap** | |
| You want an instance of your own. Install it, write ``config.yml``, author the data, | |
| moderate what users submit, put it in production. No Python programming required. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/index.rst` around lines 17 - 19, Update the “Running a Goodmap” text in
the documentation to clarify that Python programming experience is not required,
while preserving that Python 3.10 or newer is needed to run the application.
| With Poetry, in the project that will host your map: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| poetry add goodmap |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
poetry --version
poetry add --help | rg -n -- '--allow-prereleases|pre-release|prerelease' || trueRepository: Problematy/goodmap
Length of output: 200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/installation.rst relevant lines =="
sed -n '1,90p' docs/installation.rst | cat -n
echo
echo "== package metadata references to goodmap and pre-releases =="
for f in pyproject.toml poetry.lock setup.py setup.cfg MANIFEST.in README.md README.rst; do
if [ -f "$f" ]; then
echo "-- $f --"
sed -n '1,220p' "$f" | nl -ba | rg -n "goodmap|pre-release|pre_release|alpha|beta|rc|dev|classifier|dev" || true
fi
done
echo
echo "== repository files with goodmap version mentions =="
rg -n --glob '!*.pyc' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' 'goodmap|pre-release|pre_release|allow-prereleases|allow_prereleases|prereleases|allow-prerelease' .Repository: Problematy/goodmap
Length of output: 50374
🌐 Web query:
Poetry poetry add allow prereleases --allow-prereleases documentation
💡 Result:
The --allow-prereleases flag is a valid option for the poetry add command, used to allow the installation of pre-release versions of a package [1][2][3]. When running the command, you can include this flag to permit Poetry to consider pre-release versions during dependency resolution [1][4]: poetry add package-name --allow-prereleases By default, Poetry prefers stable releases and will only select a pre-release if no stable version satisfies the specified version constraint [5][6]. Using --allow-prereleases effectively tells Poetry to make no distinction between stable and pre-release versions, allowing it to select the latest version regardless of its release status [5][6]. If you need to persist this behavior for a specific dependency in your project configuration (pyproject.toml) without using the command-line flag every time, you can define the dependency with the allow-prereleases attribute [5][6]: [tool.poetry.dependencies] package-name = {version = "1.0.0a1", allow-prereleases = true} For further details, refer to the official Poetry documentation regarding the CLI commands [1] and dependency specification [5].
Citations:
- 1: https://python-poetry.org/docs/cli/
- 2: https://github.com/python-poetry/poetry/blob/f4670233/docs/cli.md
- 3: https://python-poetry.org/docs/1.8/cli/
- 4: https://python-poetry.org/docs/main/cli
- 5: https://python-poetry.org/docs/dependency-specification/
- 6: https://python-poetry.org/docs/main/dependency-specification
🌐 Web query:
Poetry 1.9 add prerelease behavior change allow prereleases
💡 Result:
In Poetry, the allow-prereleases setting functions as a tri-state configuration that controls how the dependency solver handles pre-release versions [1]. This setting allows users to explicitly manage whether or not pre-releases should be considered during resolution [2][3]. The three states for allow-prereleases are: 1. Not set (Default): Poetry will prefer stable releases. It will only select a pre-release version if no stable release satisfies the defined version constraint [2][4]. 2. false: Poetry strictly prohibits the use of pre-release versions. If no stable release meets the version requirement, dependency resolution will fail, even if a pre-release version is available that would otherwise satisfy the constraint [1][2][4]. 3. true: Poetry makes no distinction between stable and pre-release versions, allowing the solver to treat them equally during dependency resolution [1][2][4]. Historically, updates in Poetry and poetry-core have focused on improving PEP 440 compliance regarding pre-releases [5]. Specifically, changes were made to ensure that pre-releases are not implicitly allowed unless necessary (e.g., if no other stable version exists) or explicitly requested by the user [5]. The tri-state behavior ensures that setting allow-prereleases to false provides a definitive way to forbid pre-releases entirely [1].
Citations:
- 1: make
allow-prereleasesa tri-state setting to really forbid pre-releases if the setting isfalseand keep default behavior to allow pre-releases only if necessary python-poetry/poetry#9798 - 2: https://python-poetry.org/docs/dependency-specification/
- 3: https://python-poetry.org/docs/main/dependency-specification
- 4: https://github.com/python-poetry/poetry/blob/main/docs/dependency-specification.md
- 5: PEP 440 compliance: do not implicitly allow pre-releases python-poetry/poetry-core#543
Document explicit pre-release selection for Poetry.
poetry add goodmap does not document how Poetry selects the 2.0.0-alpha.N installation target. Add the supported pre-release option or constraint, for example poetry add goodmap --allow-prereleases, or goodmap = { version = "^2.0.0", allow-prereleases = true }.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/installation.rst` around lines 33 - 37, Update the Poetry installation
example in the project map setup section to explicitly enable pre-release
selection for goodmap, using the supported command-line option or an equivalent
dependency constraint with allow-prereleases enabled.
| Then ``curl http://localhost:5000/api/version`` should report the installed version: | ||
|
|
||
| .. code-block:: json | ||
|
|
||
| {"backend": "2.0.0a5"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use an example placeholder for the API version.
The command reports the installed version, but 2.0.0a5 is valid only for one release. Replace it with a placeholder or label the block as example output.
Proposed output
- {"backend": "2.0.0a5"}
+ {"backend": "<installed-version>"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Then ``curl http://localhost:5000/api/version`` should report the installed version: | |
| .. code-block:: json | |
| {"backend": "2.0.0a5"} | |
| Then ``curl http://localhost:5000/api/version`` should report the installed version: | |
| .. code-block:: json | |
| {"backend": "<installed-version>"} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/installation.rst` around lines 50 - 54, Update the API version response
example following the curl command to use a clearly generic placeholder instead
of the release-specific “2.0.0a5” value, while preserving the documented JSON
structure.
| # TODO admin API endpoints do not authenticate the caller | ||
| # Every route below is reachable by anyone who can reach the app once | ||
| # ENABLE_ADMIN_PANEL is on - reading, creating, editing and deleting locations. | ||
| # Only the /goodmap-admin *page* checks session["user"] (see goodmap.py::admin); | ||
| # the API behind it does not. CSRF protection stops a third-party site from driving | ||
| # a logged-in browser, but not a direct request. Add a session/role check here | ||
| # (e.g. a before_request on this blueprint) rather than relying on deployments to | ||
| # restrict /api/admin/ at the proxy. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Block unauthenticated access to the admin API.
At Lines 187-194, the comment identifies that enabling ENABLE_ADMIN_PANEL exposes unauthenticated read, create, update, and delete endpoints. The page-level session check does not protect direct API requests. Add a blueprint-level authorization guard that uses the application's admin or role mechanism, and return 401 or 403 before route handlers execute. Add tests for anonymous and non-admin requests.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 187-187: Complete the task associated to this "TODO" comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@goodmap/admin_api.py` around lines 187 - 194, Add a before-request
authorization guard for the admin API blueprint near the existing endpoint
definitions, reusing the application’s established session/admin role mechanism
to reject anonymous and non-admin callers with 401 or 403 before any route
handler runs. Preserve access for authorized admins and add coverage for both
anonymous and authenticated non-admin requests.
| # TODO the MAX_CONTENT_LENGTH guard below never fires, so the limit is never applied | ||
| # Flask's default config already contains MAX_CONTENT_LENGTH (set to None), so | ||
| # `"MAX_CONTENT_LENGTH" not in app.config` is always False and the 100KB cap is | ||
| # dead code - request bodies are currently unbounded. Test with `app.config.get(...) | ||
| # is None` instead, and decide how this interacts with the 5MB photo upload limit | ||
| # in core_pages(): a 100KB cap would reject every photo larger than 100KB. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Apply a finite request-body limit instead of documenting the bypass.
At Lines 191-196, the comment confirms that request bodies are currently unbounded. This leaves the application exposed to oversized-request and memory-exhaustion risk. Set a global limit that covers the 5 MB multipart upload plus overhead, then apply smaller limits to JSON endpoints if required. Do not only change the condition to app.config.get(...) is None while keeping the 100 KB value. Add tests for default, configured, and oversized requests.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 191-191: Complete the task associated to this "TODO" comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@goodmap/goodmap.py` around lines 191 - 196, Replace the ineffective
MAX_CONTENT_LENGTH guard in the application configuration with a finite global
request-body limit large enough for the 5 MB multipart upload and protocol
overhead, while preserving explicitly configured limits. Apply stricter limits
to JSON endpoints where needed, and add coverage for default, configured, and
oversized requests.
| <!-- TODO Locations tab is hardcoded to the bridges example schema | ||
| The table columns, the "Filter by Type"/"Filter by Accessibility" dropdowns, the | ||
| add/edit form and its /api/category/accessible_by fetch all assume the fields | ||
| name, position, type_of_place and accessible_by. Deployments with any other | ||
| schema cannot use this tab: custom fields are neither shown nor editable, and | ||
| renderLocations() throws on loc.accessible_by.join() when the field is absent. | ||
| Drive the columns and the form from the location schema instead of naming fields | ||
| literally - note goodmap.py::admin does not currently pass location_schema to | ||
| this template (only the /map view gets it), so that has to be added, or the tab | ||
| can read /api/categories-full at runtime. The Suggestions and Reports tabs are | ||
| schema-agnostic and need no change. --> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve or explicitly gate the schema mismatch.
At Lines 39-49, the comment describes a functional failure, but the Locations tab remains enabled for custom schemas. Render the columns and form from the location schema, or disable the tab when required fields are absent and show a clear message. Add an integration test for a schema without accessible_by.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 39-39: Complete the task associated to this "TODO" comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@goodmap/templates/goodmap-admin.html` around lines 39 - 49, Resolve the
schema mismatch in the Locations tab by driving its columns, filters, add/edit
form, and rendering logic from the location schema passed by goodmap.py::admin,
including safe handling when accessible_by is absent; alternatively, disable the
tab when required fields are missing and display a clear message. Ensure the
/api/category/accessible_by request and renderLocations() do not assume bridges
fields, and add an integration test covering a schema without accessible_by.
| # TODO verify-json-data is broken: goodmap.data_validator does not exist | ||
| # The target always fails with "No module named goodmap.data_validator". Either restore | ||
| # the module or reimplement it: build a model with | ||
| # data_models.location.create_location_model(location_obligatory_fields, categories) | ||
| # from the file's own "map" section and model_validate() every point, exiting non-zero | ||
| # on the first failure. Docs (docs/data-source.rst) currently ship a standalone script | ||
| # as a workaround - fold it back in here once this works. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fix or remove the guaranteed-failing validation workflow.
At Lines 40-46, the comment confirms that verify-json-data always fails because goodmap.data_validator is missing, but the target remains available to contributors. Implement the module before relying on this target, or remove the target and update docs/data-source.rst and CI callers. Add tests for valid and invalid JSON input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 40 - 46, Fix the guaranteed-failing verify-json-data
workflow by implementing the missing goodmap.data_validator module, or remove
the target and update docs/data-source.rst plus all CI callers accordingly. If
implementing it, build the model via data_models.location.create_location_model
using the file’s map section, validate every point, and exit non-zero on the
first invalid input. Add coverage for both valid and invalid JSON data.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/admin-panel.rst`:
- Around line 107-113: Update the google_hosted_json_file moderation write
handlers for accept, reject, edit, and delete to return an explicit
unsupported-operation error instead of silently succeeding, and ensure the admin
panel displays that error to moderators while preserving read-only behavior.
- Around line 23-34: Enforce authorization at the /api/admin/ endpoint boundary
for every point creation, editing, and deletion handler by requiring a valid
authenticated session and appropriate admin role, or reject requests when
trusted authentication is not configured. Do not treat CSRF validation or
FAKE_LOGIN as authorization; ensure scripted callers must provide valid
authorization, and update the admin documentation to reflect the enforced
behavior.
In `@docs/data-source.rst`:
- Around line 34-35: Update the field count in the data description near the
``data`` list to state six fields instead of four, matching the documented
fields: uuid, name, position, type_of_place, accessible_by, and remark.
- Around line 28-29: Update the data-source documentation to list
location_obligatory_fields as structurally required for JSON backends, and
revise the minimal schema and examples to include it with the appropriate value,
such as an empty list when no fields are obligatory.
- Around line 125-132: Update the visible_data documentation to state that only
fields listed in neither visible_data nor meta_data are excluded from the
frontend response. Preserve the existing descriptions of popup body ordering and
the separate metadata object.
- Around line 151-152: Update the category documentation around the
category-validation guarantee to state that category values constrain matching
fields only when those fields are included in location_obligatory_fields.
Alternatively, revise the example so it requires categorized fields to be listed
there, without implying that a category key alone validates every matching point
field.
- Around line 227-240: The category contract is inconsistent for the
threshold-filtered speed_limit field: align the Location data model, category
filtering implementation, and this documentation example so speed_limit uses
numeric values and storage consistently, while preserving the threshold filter
behavior and updating any validation or serialization that assumes all category
values are strings.
In `@docs/http-api.rst`:
- Around line 4-5: Update the API introduction near the response-format
statement to say responses are JSON by default, except when an endpoint
documents an empty response such as the 204 DELETE location response. Apply the
same wording change to the corresponding repeated introduction section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a13576ed-532d-4b9e-95a8-6a34ab35dc74
📒 Files selected for processing (5)
docs/admin-panel.rstdocs/configuration.rstdocs/data-source.rstdocs/deployment.rstdocs/http-api.rst
| .. danger:: | ||
|
|
||
| **The admin API does not check who is calling.** The page checks for a session; the | ||
| endpoints behind it do not. With ``ENABLE_ADMIN_PANEL`` on, anyone who can reach the | ||
| app and obtain a CSRF token can create, edit and delete points — no login needed. Read | ||
| :ref:`deployment-admin` before enabling this on a public deployment. | ||
|
|
||
| .. warning:: | ||
|
|
||
| ``FAKE_LOGIN`` lets anyone into the admin area by picking a role, with no password. It | ||
| is a local-development convenience. Never combine it with ``ENABLE_ADMIN_PANEL`` on a | ||
| reachable host. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Enforce authorization at the admin API boundary.
The documentation states that /api/admin/ permits point creation, editing, and deletion without a login. A CSRF token is not authorization. FAKE_LOGIN is also unsafe on a reachable host.
Add session and role checks to every admin endpoint, or fail closed when trusted authentication is not configured. Do not rely only on deployment warnings. State that scripted callers need authorization, not only a CSRF token.
Also applies to: 117-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/admin-panel.rst` around lines 23 - 34, Enforce authorization at the
/api/admin/ endpoint boundary for every point creation, editing, and deletion
handler by requiring a valid authenticated session and appropriate admin role,
or reject requests when trusted authentication is not configured. Do not treat
CSRF validation or FAKE_LOGIN as authorization; ensure scripted callers must
provide valid authorization, and update the admin documentation to reflect the
enforced behavior.
| * - ``json_file`` | ||
| - Works. Writes are atomic. Single-process only — see :ref:`deployment-workers`. | ||
| * - MongoDB | ||
| - Works, and is the right choice for concurrent moderators. | ||
| * - ``google_hosted_json_file`` | ||
| - **Read-only.** Accept, reject, edit and delete all silently do nothing. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not allow silent success for read-only writes.
If google_hosted_json_file ignores Accept, reject, edit, and delete requests, moderators can believe that moderation succeeded while the map remains unchanged. Return an explicit unsupported-operation error and surface it in the panel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/admin-panel.rst` around lines 107 - 113, Update the
google_hosted_json_file moderation write handlers for accept, reject, edit, and
delete to return an explicit unsupported-operation error instead of silently
succeeding, and ensure the admin panel displays that error to moderators while
preserving read-only behavior.
| Only ``data`` and ``categories`` are structurally required; ``suggestions`` and | ||
| ``reports`` are created by the app as users submit things. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Document location_obligatory_fields as required for JSON backends.
With USE_LAZY_LOADING enabled, startup reads location_obligatory_fields directly. The JSON backends do not provide a fallback, so omitting this key raises KeyError before the application serves requests. (github.com)
Update the minimal schema and examples, or add a [] fallback in the backend loaders.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/data-source.rst` around lines 28 - 29, Update the data-source
documentation to list location_obligatory_fields as structurally required for
JSON backends, and revise the minimal schema and examples to include it with the
appropriate value, such as an empty list when no fields are obligatory.
Source: MCP tools
| ``data`` is the list of points. Each one is a free-form object with four fields Goodmap | ||
| cares about: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the field count.
The example lists six fields, not four: uuid, name, position, type_of_place, accessible_by, and remark.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/data-source.rst` around lines 34 - 35, Update the field count in the
data description near the ``data`` list to state six fields instead of four,
matching the documented fields: uuid, name, position, type_of_place,
accessible_by, and remark.
| ``visible_data`` | ||
| Field names shown in the marker popup's body, in the order given. **This is also a | ||
| privacy boundary**: a field not listed here is never sent to the frontend by | ||
| ``/api/location/<uuid>``, so internal fields can live in the data safely. | ||
|
|
||
| ``meta_data`` | ||
| Field names returned in the popup's separate ``metadata`` object, for data that is | ||
| needed but not part of the visible body — typically ``uuid``. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fix the visible_data privacy boundary.
A field can be absent from visible_data and still be returned through meta_data. The current wording incorrectly says that such a field is never sent to the frontend.
Use “fields listed in neither visible_data nor meta_data are not returned.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/data-source.rst` around lines 125 - 132, Update the visible_data
documentation to state that only fields listed in neither visible_data nor
meta_data are excluded from the frontend response. Preserve the existing
descriptions of popup body ordering and the separate metadata object.
| Categories do double duty: they define the filters **and** constrain what values the | ||
| matching field may hold, so a point with ``"accessible_by": ["boats"]`` is rejected. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Qualify the category-validation guarantee.
Category validators are created for fields listed in location_obligatory_fields; a category key alone does not validate every matching point field. (github.com)
Require every categorized field to appear in location_obligatory_fields, or state this dependency explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/data-source.rst` around lines 151 - 152, Update the category
documentation around the category-validation guarantee to state that category
values constrain matching fields only when those fields are included in
location_obligatory_fields. Alternatively, revise the example so it requires
categorized fields to be listed there, without implying that a category key
alone validates every matching point field.
Source: MCP tools
| { | ||
| "categories": { | ||
| "accessible_by": ["bikes", "cars", "pedestrians"], | ||
| "type_of_place": ["big bridge", "small bridge"], | ||
| "is_free": ["true", "false"], | ||
| "speed_limit": ["10", "30", "50"], | ||
| "amenities": ["lighting", "benches", "toilets"] | ||
| }, | ||
| "categories_filter_mode": { | ||
| "accessible_by": "or", | ||
| "type_of_place": "exclusive", | ||
| "is_free": "boolean", | ||
| "speed_limit": "threshold", | ||
| "amenities": "and" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the threshold type contract.
The example defines speed_limit values as strings, but the MongoDB section requires numeric storage. The implementation also models category values as strings, so changing only the example may break validation. (github.com)
Align the model, filtering code, and documentation before publishing this example.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/data-source.rst` around lines 227 - 240, The category contract is
inconsistent for the threshold-filtered speed_limit field: align the Location
data model, category filtering implementation, and this documentation example so
speed_limit uses numeric values and storage consistently, while preserving the
threshold filter behavior and updating any validation or serialization that
assumes all category values are strings.
Source: MCP tools
| Everything the map UI does, it does through this API — so anything the UI can do, your own | ||
| client can do too. All responses are JSON. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not promise JSON for every API response.
DELETE /api/admin/locations/<uuid> returns 204 with no body. Change the introduction to state that responses are JSON unless an endpoint documents an empty response.
Also applies to: 380-381
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/http-api.rst` around lines 4 - 5, Update the API introduction near the
response-format statement to say responses are JSON by default, except when an
endpoint documents an empty response such as the 204 DELETE location response.
Apply the same wording change to the corresponding repeated introduction
section.
|
1 participant
Summary by CodeRabbit