feat(tests): improve test coverage#234
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 15 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds route guards for login/admin access, rewires protected routes to use them, renames change-username translation keys, and expands Playwright coverage with new helpers, page objects, docs, and domain tests. ChangesAuthentication, translations, and E2E coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 4
🤖 Fix all issues with AI agents
In `@app/routes/post.py`:
- Around line 52-59: The redirect responses in app/routes/post.py currently
force a 301 status (e.g., the redirect(...) , 301 returned after the permission
check and other POST actions), which is incorrect for POST form actions; remove
the explicit ", 301" status overrides on the redirect(...) calls (leave the
default 302 or use 303 for POST→GET if you prefer) for the permission-denied and
post/comment actions referenced by redirect(url_for("post.post", url_id=url_id))
and the other three identical redirects so they no longer return 301.
- Around line 64-84: Guard against missing form fields and validate comment
submission: replace direct index access to request.form["comment_id"] and
request.form["comment"] with safe access (request.form.get(...)) and ensure the
branch only proceeds when the form indicates a comment submission and the
CommentForm (or equivalent) validates. Specifically, in the block handling
"comment_delete_button" use comment_id = request.form.get("comment_id") and bail
out (redirect) if it's None or invalid before calling Comment.query.get; and for
comment creation ensure you check for the comment-submit indicator (or validate
CommentForm.is_submitted()/validate()) before reading
request.form.get("comment"), validate/escape the text, and only then call
create_comment (or the existing create path), keeping permission checks and
delete_comment(comment_id) unchanged.
In `@tests/e2e/account/test_dashboard.py`:
- Around line 33-35: Two tests that require an authenticated session are missing
the `@pytest.mark.auth` marker: add `@pytest.mark.auth` above the test functions
test_dashboard_shows_user_posts_and_comments and test_dashboard_can_delete_post
so they are included in auth-filtered runs; ensure pytest is imported in the
file if not already and place the decorator directly above each def (keeping the
existing _login usage and test parameters intact).
In `@tests/e2e/post/test_post.py`:
- Around line 211-216: Replace the unnecessary BasePage instantiation by calling
the inherited method on PostPage directly: remove creation of BasePage(page,
flask_server["base_url"]) and the variable base_page, then call
post_page.expect_success_flash() instead of base_page.expect_success_flash();
ensure you keep the subsequent page.wait_for_url and
post_page.expect_comment_visible(comment_text) calls unchanged so the flow and
assertions remain intact.
🧹 Nitpick comments (6)
tests/e2e/helpers/database_helpers.py (1)
160-167: Optional: extract a context-manager to reduce connection boilerplate.Every helper repeats the same
conn = get_db_connection(...)/try/finally: conn.close()pattern. A small context manager would remove ~4 lines per function and make the pattern less error-prone if new helpers are added.Example context manager
from contextlib import contextmanager `@contextmanager` def db_connection(db_path: str, row_factory=None): conn = get_db_connection(db_path) if row_factory: conn.row_factory = row_factory try: yield conn, conn.cursor() finally: conn.close()Usage:
def get_post_by_title(db_path: str, title: str) -> dict | None: with db_connection(db_path, row_factory=sqlite3.Row) as (conn, cursor): cursor.execute("SELECT * FROM posts WHERE title = ?", (title,)) row = cursor.fetchone() return dict(row) if row else NoneAlso applies to: 266-268, 299-300
tests/e2e/home/test_home.py (1)
13-14:_suffix()is duplicated across multiple test files.This same helper appears in
test_home.py,test_search.py,test_category.py, andtest_admin.py. Consider extracting it totests/e2e/helpers/(e.g., alongsidetest_data.py) to avoid repetition.tests/e2e/pages/post_page.py (1)
35-37:get_views_countwill raise an unhandledValueErroron non-numeric text.If the rendered view count includes formatting like commas (
"1,234") or abbreviations ("1.2K"),int(views_text)will throw. For a test page object this is likely fine in practice, but worth noting if the app ever changes its display format.tests/e2e/post/test_post.py (2)
39-43:_loginhelper is duplicated across three test files.This identical helper appears in
test_post.py,test_dashboard.py, andtest_account_settings.py. Consider extracting it to a shared conftest fixture or a common test helper module (e.g.,tests/e2e/helpers/auth_helpers.py) to reduce duplication.
145-162:@pytest.mark.authon tests that don't require user authentication may be misleading.
test_post_url_redirects_to_canonical_slugandtest_post_views_increment_when_post_is_opened(lines 145–184) create posts via DB helpers and access them without logging in. If theauthmarker is used to filter authentication-related tests, these may not belong under it. If the post route itself requires authentication, then the tests would fail for a different reason (redirect to login rather than testing routing/views). Consider whether a different marker (or no marker) is more appropriate.tests/e2e/account/test_static_pages.py (1)
17-18: CSS framework class selectors (.btn-primary,.badge-ghost) may be fragile.These are DaisyUI/Tailwind utility classes that could change across UI framework versions. Consider using
data-testidattributes or more semantic selectors if the project adopts that pattern. Not critical for smoke tests, just a resilience consideration.
| if ( | ||
| post.author != session["username"] | ||
| and session.get("user_role") != "admin" | ||
| ): | ||
| Log.error( | ||
| f'User: "{session["username"]}" tried to delete post "{url_id}" without permission', | ||
| ) | ||
| return redirect(url_for("post.post", url_id=url_id)), 301 |
There was a problem hiding this comment.
HTTP 301 (Moved Permanently) is semantically incorrect for action responses.
return redirect(url), 301 overrides the redirect status to 301. These are all responses to POST form submissions (delete post, delete comment, add comment), not permanent URL relocations. Browsers and proxies cache 301 redirects, meaning a browser might skip hitting the server entirely on subsequent requests to the same URL, leading to stale or broken behavior.
Use the default 302 (or 303 for POST→GET) instead:
Proposed fix — drop the 301 overrides
- return redirect(url_for("post.post", url_id=url_id)), 301
+ return redirect(url_for("post.post", url_id=url_id))Apply the same change to all four occurrences (Lines 59, 70, 79, 82, and 108). For example:
- return redirect(url_for("post.post", url_id=url_id)), 301
+ return redirect(url_for("post.post", url_id=url_id))
- delete_comment(comment_id)
- return redirect(url_for("post.post", url_id=url_id)), 301
+ delete_comment(comment_id)
+ return redirect(url_for("post.post", url_id=url_id))
- return redirect(url_for("post.post", url_id=url_id)), 301
+ return redirect(url_for("post.post", url_id=url_id))Also applies to: 68-70, 79-82, 108-108
🤖 Prompt for AI Agents
In `@app/routes/post.py` around lines 52 - 59, The redirect responses in
app/routes/post.py currently force a 301 status (e.g., the redirect(...) , 301
returned after the permission check and other POST actions), which is incorrect
for POST form actions; remove the explicit ", 301" status overrides on the
redirect(...) calls (leave the default 302 or use 303 for POST→GET if you
prefer) for the permission-denied and post/comment actions referenced by
redirect(url_for("post.post", url_id=url_id)) and the other three identical
redirects so they no longer return 301.
| if "comment_delete_button" in request.form: | ||
| delete_comment(request.form["comment_id"]) | ||
| comment_id = request.form["comment_id"] | ||
| comment = Comment.query.get(comment_id) | ||
|
|
||
| if not comment: | ||
| Log.error(f'Comment: "{comment_id}" not found for post "{url_id}"') | ||
| return redirect(url_for("post.post", url_id=url_id)), 301 | ||
|
|
||
| if ( | ||
| comment.username != session["username"] | ||
| and session.get("user_role") != "admin" | ||
| ): | ||
| Log.error( | ||
| f'User: "{session["username"]}" tried to delete comment "{comment_id}" without permission', | ||
| ) | ||
| return redirect(url_for("post.post", url_id=url_id)), 301 | ||
|
|
||
| delete_comment(comment_id) | ||
| return redirect(url_for("post.post", url_id=url_id)), 301 | ||
|
|
||
| comment_text = escape(request.form["comment"]) |
There was a problem hiding this comment.
Missing guard for the comment creation fallthrough and missing KeyError handling.
Two concerns in this block:
-
Line 65:
request.form["comment_id"]will raise aKeyErrorif a malicious or malformed request omits the field. Userequest.form.get("comment_id")with a guard, or catch the exception. -
Lines 84+: If a POST request reaches this point without any recognized button and without a
"comment"key in the form, line 84 (request.form["comment"]) will raise a 500. There's no explicitelifor guard ensuring this is actually a comment submission. ACommentFormis instantiated (line 29) but never validated.
Proposed fix — use safe form access and validate before creating a comment
if "comment_delete_button" in request.form:
- comment_id = request.form["comment_id"]
+ comment_id = request.form.get("comment_id")
+ if not comment_id:
+ return redirect(url_for("post.post", url_id=url_id))
comment = Comment.query.get(comment_id)For the comment creation block, consider validating the form:
- comment_text = escape(request.form["comment"])
+ if not form.validate():
+ return redirect(url_for("post.post", url_id=url_id))
+ comment_text = escape(form.comment.data)📝 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.
| if "comment_delete_button" in request.form: | |
| delete_comment(request.form["comment_id"]) | |
| comment_id = request.form["comment_id"] | |
| comment = Comment.query.get(comment_id) | |
| if not comment: | |
| Log.error(f'Comment: "{comment_id}" not found for post "{url_id}"') | |
| return redirect(url_for("post.post", url_id=url_id)), 301 | |
| if ( | |
| comment.username != session["username"] | |
| and session.get("user_role") != "admin" | |
| ): | |
| Log.error( | |
| f'User: "{session["username"]}" tried to delete comment "{comment_id}" without permission', | |
| ) | |
| return redirect(url_for("post.post", url_id=url_id)), 301 | |
| delete_comment(comment_id) | |
| return redirect(url_for("post.post", url_id=url_id)), 301 | |
| comment_text = escape(request.form["comment"]) | |
| if "comment_delete_button" in request.form: | |
| comment_id = request.form.get("comment_id") | |
| if not comment_id: | |
| return redirect(url_for("post.post", url_id=url_id)) | |
| comment = Comment.query.get(comment_id) | |
| if not comment: | |
| Log.error(f'Comment: "{comment_id}" not found for post "{url_id}"') | |
| return redirect(url_for("post.post", url_id=url_id)), 301 | |
| if ( | |
| comment.username != session["username"] | |
| and session.get("user_role") != "admin" | |
| ): | |
| Log.error( | |
| f'User: "{session["username"]}" tried to delete comment "{comment_id}" without permission', | |
| ) | |
| return redirect(url_for("post.post", url_id=url_id)), 301 | |
| delete_comment(comment_id) | |
| return redirect(url_for("post.post", url_id=url_id)), 301 | |
| if not form.validate(): | |
| return redirect(url_for("post.post", url_id=url_id)) | |
| comment_text = escape(form.comment.data) |
🤖 Prompt for AI Agents
In `@app/routes/post.py` around lines 64 - 84, Guard against missing form fields
and validate comment submission: replace direct index access to
request.form["comment_id"] and request.form["comment"] with safe access
(request.form.get(...)) and ensure the branch only proceeds when the form
indicates a comment submission and the CommentForm (or equivalent) validates.
Specifically, in the block handling "comment_delete_button" use comment_id =
request.form.get("comment_id") and bail out (redirect) if it's None or invalid
before calling Comment.query.get; and for comment creation ensure you check for
the comment-submit indicator (or validate CommentForm.is_submitted()/validate())
before reading request.form.get("comment"), validate/escape the text, and only
then call create_comment (or the existing create path), keeping permission
checks and delete_comment(comment_id) unchanged.
| def test_dashboard_shows_user_posts_and_comments( | ||
| self, page, flask_server, test_user, db_path | ||
| ): |
There was a problem hiding this comment.
Inconsistent @pytest.mark.auth usage — two tests that require login are missing the marker.
test_dashboard_shows_user_posts_and_comments (line 33) and test_dashboard_can_delete_post (line 96) both log in via _login and exercise authenticated flows, but lack the @pytest.mark.auth marker that other auth-dependent tests in this PR consistently use. If the marker is used for filtering or CI gating, these tests would be excluded from auth test runs.
♻️ Suggested fix
+ `@pytest.mark.auth`
def test_dashboard_shows_user_posts_and_comments(
self, page, flask_server, test_user, db_path
):+ `@pytest.mark.auth`
def test_dashboard_can_delete_post(self, page, flask_server, test_user, db_path):Also applies to: 96-97
🤖 Prompt for AI Agents
In `@tests/e2e/account/test_dashboard.py` around lines 33 - 35, Two tests that
require an authenticated session are missing the `@pytest.mark.auth` marker: add
`@pytest.mark.auth` above the test functions
test_dashboard_shows_user_posts_and_comments and test_dashboard_can_delete_post
so they are included in auth-filtered runs; ensure pytest is imported in the
file if not already and place the decorator directly above each def (keeping the
existing _login usage and test parameters intact).
| post_page.add_comment(comment_text) | ||
|
|
||
| base_page = BasePage(page, flask_server["base_url"]) | ||
| base_page.expect_success_flash() | ||
| page.wait_for_url("**/post/**", timeout=5000) | ||
| post_page.expect_comment_visible(comment_text) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unnecessary BasePage instantiation — PostPage inherits expect_success_flash.
PostPage extends BasePage, so post_page.expect_success_flash() works directly. No need to create a separate BasePage instance.
♻️ Suggested fix
post_page.add_comment(comment_text)
- base_page = BasePage(page, flask_server["base_url"])
- base_page.expect_success_flash()
+ post_page.expect_success_flash()
page.wait_for_url("**/post/**", timeout=5000)
post_page.expect_comment_visible(comment_text)🤖 Prompt for AI Agents
In `@tests/e2e/post/test_post.py` around lines 211 - 216, Replace the unnecessary
BasePage instantiation by calling the inherited method on PostPage directly:
remove creation of BasePage(page, flask_server["base_url"]) and the variable
base_page, then call post_page.expect_success_flash() instead of
base_page.expect_success_flash(); ensure you keep the subsequent
page.wait_for_url and post_page.expect_comment_visible(comment_text) calls
unchanged so the flow and assertions remain intact.
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@app/routes/admin_panel_users.py`:
- Around line 23-36: The POST handlers call delete_user(...) and
change_user_role(...) but ignore their return values (which may be Flask
redirect responses); update the POST branch in the request.method == "POST"
block to capture the result of delete_user(request.form["username"]) and
change_user_role(request.form["username"]) and, if the result is a
Response/redirect, immediately return it (otherwise continue). Ensure you still
log with Log.info(...) before calling those functions and propagate any redirect
returned by those functions so redirects from delete_user and change_user_role
are not lost.
- Around line 41-54: Remove the password field from the tuple built from
users_objects in the users list comprehension (currently includes u.password) so
templates do not receive password hashes; then update the corresponding index
references in admin_panel_users.html to account for the shifted tuple positions
(change user[4]→user[3], user[5]→user[4], user[6]→user[5], user[7]→user[6]).
Locate the list construction around the users = [ (... ) for u in users_objects
] in admin_panel_users.py and the index uses in admin_panel_users.html and
adjust them accordingly.
In `@app/routes/change_password.py`:
- Around line 56-97: The three independent conditionals after
encryption.verify(old_password, user.password) can trigger multiple flash
messages; change them to an exclusive control flow (use if / elif / elif or
return early) so only one branch runs: check old_password == password first
(flash "same" and return/redirect), else if password != password_confirm (flash
"match" and return/redirect), else (passwords different from old and match each
other) set user.password via encryption.hash(password), call
db.session.commit(), call Log.success(...), clear session, flash "success" using
session.get("language", "en") and then return redirect("/login/redirect=&").
Ensure you update the block containing encryption.verify, flash_message,
user.password, db.session.commit, Log.success, session, and redirect
accordingly.
In `@app/routes/change_username.py`:
- Around line 49-65: Post and Comment rows (Post.query.filter_by(...) and
Comment.query.filter_by(...)) and db.session.commit() run even when the User
lookup fails; move the Post and Comment update calls and the db.session.commit()
inside the same if user: guard that wraps user.username = new_username (or
replace the guard so the entire update block is conditional on user being
truthy), and add handling for the case when
User.query.filter_by(username=old_username).first() returns none (e.g.,
return/abort or skip updates) so Post, Comment and commit only execute when a
corresponding User exists.
In `@app/routes/create_post.py`:
- Around line 42-50: CreatePostForm is instantiated but its validators are
bypassed because code reads values directly from request.form; call
form.validate() (or use form.validate_on_submit()) and, on success, use
form.post_title.data, form.post_tags.data, form.post_abstract.data,
form.post_content.data, form.post_category.data instead of request.form[...] so
the min/max validators on post_abstract and post_content run server-side; if
validate() fails, return/flash form.errors and re-render the form. Keep using
request.files["post_banner"] for the file but only process it after form
validation.
In `@app/routes/dashboard.py`:
- Around line 34-37: The redirect after a POST in app/routes/dashboard.py
currently returns redirect(url_for("dashboard.dashboard", username=username)),
301 which is incorrect; change it to use a non-permanent redirect (either return
redirect(url_for("dashboard.dashboard", username=username)) to get a 302 or
explicitly return redirect(url_for("dashboard.dashboard", username=username),
code=303) to enforce a POST→GET See Other). Update the return in the handler
that issues this redirect (the call site using
redirect(url_for("dashboard.dashboard", username=username))) accordingly.
- Around line 76-80: session.get("language") can be None causing
translation_file to become "./translations/None.json" and raise
FileNotFoundError; update the code around session.get("language") and
translation_file so you use a safe default (e.g., "en") when
session.get("language") returns None or an empty value. Concretely, replace the
raw session.get("language") use with something like language =
session.get("language") or "en" (or session.get("language", "en")), then build
translation_file and proceed to open/load; ensure the change touches the
language variable, translation_file, and the with open(...) load(...) sequence
so it never attempts to open "None.json".
In `@app/routes/edit_post.py`:
- Line 60: The current code accesses request.files["post_banner"] directly which
can raise KeyError if the field is missing; change it to use
request.files.get("post_banner") and guard before calling .read(): retrieve the
file object into a variable (e.g., post_banner_file =
request.files.get("post_banner")), check if post_banner_file is truthy (and
optionally has a filename) and only then call post_banner_file.read() to assign
post_banner; otherwise set post_banner to None or handle the missing-file branch
(validation error/flash/redirect) inside the same function handling the form
submission.
In `@app/routes/verify_user.py`:
- Line 52: The module-level variable verification_code is a concurrency hazard
because it allows concurrent flows to clobber each other's codes; replace this
global by persisting per-user codes (e.g., attach the code to the user's session
or to your user verification DB record) instead of using verification_code.
Update any functions or routes that read/write verification_code (e.g., the
route handler that generates/sends the code and the handler that validates it)
to read from and write to the chosen per-user store (session key or a
Verification table keyed by user_id/email), ensure proper expiration timestamps
and single-use semantics, and remove the global declaration so all reads/writes
are scoped to the user context.
In `@app/utils/route_guards.py`:
- Around line 10-44: The decorator uses session["username"] which conflicts with
the coding guideline's session["userName"] but the rest of the codebase
consistently uses the lowercase "username"; resolve the mismatch by choosing one
canonical key and making code + docs consistent: either (A) change this
decorator to use session.get("userName") and perform a project-wide replace of
session["username"] -> session["userName"] (update any tests and auth code that
read/write the session), or (B) accept the existing lowercase key by updating
the coding guideline and any docs to declare "username" as the canonical session
key; locate the decorator function login_required and the other auth read/write
sites to apply the chosen consistent change.
In `@tests/README.md`:
- Around line 186-190: In the CI section of tests/README.md update the product
name to use correct capitalization: change any occurrence of "github actions" or
"github" to "GitHub Actions" (and "GitHub" where used alone) so the CI heading
and the sentence "E2E tests run in GitHub Actions
(`.github/workflows/e2e-tests.yaml`) on:" use the official product casing.
🧹 Nitpick comments (7)
app/routes/admin_panel_posts.py (1)
15-15: Consider removing"POST"from allowed methods.This route only queries and renders data — there's no
request.method == "POST"handling. If the template doesn't submit forms to this endpoint, the method list can be narrowed to["GET"].app/routes/change_profile_picture.py (1)
41-62: Form validation is bypassed — rawrequest.formaccess can raiseKeyError.
request.form["new_profile_picture_seed"]on line 42 is accessed directly without callingform.validate(). If the field is missing from the POST body, this raises a400 Bad Request. Consider validating via the form object first:♻️ Suggested improvement
if request.method == "POST": - new_profile_picture_seed = request.form["new_profile_picture_seed"] + if not form.validate(): + return render_template("change_profile_picture.html", form=form) + + new_profile_picture_seed = form.new_profile_picture_seed.dataapp/routes/account_settings.py (1)
18-20: Return value fromdelete_useris discarded.
delete_user()(inapp/utils/delete.py) returns aredirect(...)response, but it's ignored here — the route always returns its ownredirect("/"). The side effects (DB deletion,session.clear(), flash) still fire, so this works for the self-deletion case. However, silently discarding the response is fragile ifdelete_user's contract changes.Consider using the return value:
♻️ Suggested improvement
if request.method == "POST": - delete_user(user.username) - return redirect("/") + return delete_user(user.username)app/routes/verify_user.py (1)
85-87: Redundantif user:check — user is guaranteed to exist.Line 44–47 already verifies the user exists and returns early if not. The
if user:guard on Line 87 is dead code.♻️ Suggested cleanup
elif code_sent == "false": if request.method == "POST": - if user: - context = ssl.create_default_context() + context = ssl.create_default_context()(and dedent the entire block below accordingly)
tests/e2e/account/test_change_password_flow.py (1)
14-18:_loginhelper is duplicated across multiple E2E test files.The AI summary notes identical
_loginhelpers intest_account_settings.pyandtest_profile_and_preferences.py. Consider extracting this into a shared fixture or utility in a conftest/helpers module to reduce duplication as the test suite grows.app/routes/dashboard.py (1)
44-60: Fragile positional list representation for posts with magic index access.Posts are stored as plain lists and accessed by magic indices (e.g.,
post[9]for category on Line 83). This is error-prone—any change to the field order silently breaks the category translation and any template code that consumes these lists. Consider using dictionaries or named tuples instead.Also applies to: 82-83
tests/e2e/admin/test_admin.py (1)
19-28:_suffix()and_login()are duplicated across test files.These helpers appear identically in
test_profile_and_preferences.pyas well. Consider extracting them into a shared test utility (e.g.,tests/e2e/helpers/) to reduce duplication.
| if request.method == "POST": | ||
| if "user_delete_button" in request.form: | ||
| Log.info( | ||
| f"Admin: {session['username']} deleted user: {request.form['username']}" | ||
| ) | ||
|
|
||
| Log.info(f"Rendering admin_panel_users.html: params: users={len(users)}") | ||
| delete_user(request.form["username"]) | ||
|
|
||
| return render_template( | ||
| "admin_panel_users.html", | ||
| users=users, | ||
| page=page, | ||
| total_pages=total_pages, | ||
| ) | ||
| else: | ||
| Log.error( | ||
| f"{request.remote_addr} tried to reach user admin panel without being admin" | ||
| if "user_role_change_button" in request.form: | ||
| Log.info( | ||
| f"Admin: {session['username']} changed {request.form['username']}'s role" | ||
| ) | ||
|
|
||
| return redirect("/") | ||
| else: | ||
| Log.error( | ||
| f"{request.remote_addr} tried to reach user admin panel being logged in" | ||
| change_user_role(request.form["username"]) |
There was a problem hiding this comment.
Return values from delete_user and change_user_role are silently discarded — redirects won't fire.
Both delete_user() and change_user_role() can return redirect(...) responses (see app/utils/delete.py lines 67–86 and app/utils/change_user_role.py lines 25–27). Since their return values are not propagated here, the code falls through to the query/render block on lines 38–63. This means:
- After deleting a user, the flash message fires but the page re-renders (potentially showing the deleted user if the DB commit already happened, or crashing if it didn't).
- If an admin demotes themselves, the
redirect("/")fromchange_user_roleis lost — the page re-renders, and subsequent requests will fail because the@admin_requiredguard will reject the now-non-admin user.
🐛 Proposed fix — propagate redirect responses
if request.method == "POST":
if "user_delete_button" in request.form:
Log.info(
f"Admin: {session['username']} deleted user: {request.form['username']}"
)
- delete_user(request.form["username"])
+ result = delete_user(request.form["username"])
+ if result:
+ return result
- if "user_role_change_button" in request.form:
+ elif "user_role_change_button" in request.form:
Log.info(
f"Admin: {session['username']} changed {request.form['username']}'s role"
)
- change_user_role(request.form["username"])
+ result = change_user_role(request.form["username"])
+ if result:
+ return result📝 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.
| if request.method == "POST": | |
| if "user_delete_button" in request.form: | |
| Log.info( | |
| f"Admin: {session['username']} deleted user: {request.form['username']}" | |
| ) | |
| Log.info(f"Rendering admin_panel_users.html: params: users={len(users)}") | |
| delete_user(request.form["username"]) | |
| return render_template( | |
| "admin_panel_users.html", | |
| users=users, | |
| page=page, | |
| total_pages=total_pages, | |
| ) | |
| else: | |
| Log.error( | |
| f"{request.remote_addr} tried to reach user admin panel without being admin" | |
| if "user_role_change_button" in request.form: | |
| Log.info( | |
| f"Admin: {session['username']} changed {request.form['username']}'s role" | |
| ) | |
| return redirect("/") | |
| else: | |
| Log.error( | |
| f"{request.remote_addr} tried to reach user admin panel being logged in" | |
| change_user_role(request.form["username"]) | |
| if request.method == "POST": | |
| if "user_delete_button" in request.form: | |
| Log.info( | |
| f"Admin: {session['username']} deleted user: {request.form['username']}" | |
| ) | |
| result = delete_user(request.form["username"]) | |
| if result: | |
| return result | |
| elif "user_role_change_button" in request.form: | |
| Log.info( | |
| f"Admin: {session['username']} changed {request.form['username']}'s role" | |
| ) | |
| result = change_user_role(request.form["username"]) | |
| if result: | |
| return result |
🤖 Prompt for AI Agents
In `@app/routes/admin_panel_users.py` around lines 23 - 36, The POST handlers call
delete_user(...) and change_user_role(...) but ignore their return values (which
may be Flask redirect responses); update the POST branch in the request.method
== "POST" block to capture the result of delete_user(request.form["username"])
and change_user_role(request.form["username"]) and, if the result is a
Response/redirect, immediately return it (otherwise continue). Ensure you still
log with Log.info(...) before calling those functions and propagate any redirect
returned by those functions so redirects from delete_user and change_user_role
are not lost.
| users = [ | ||
| ( | ||
| u.user_id, | ||
| u.username, | ||
| u.email, | ||
| u.password, | ||
| u.profile_picture, | ||
| u.role, | ||
| u.points, | ||
| u.time_stamp, | ||
| u.is_verified, | ||
| ) | ||
| for u in users_objects | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(admin_panel|routes)" | head -20Repository: DogukanUrker/FlaskBlog
Length of output: 603
🏁 Script executed:
fd 'admin_panel_users' --type fRepository: DogukanUrker/FlaskBlog
Length of output: 134
🏁 Script executed:
cat -n app/routes/admin_panel_users.py 2>/dev/null | head -60Repository: DogukanUrker/FlaskBlog
Length of output: 2117
🏁 Script executed:
cat -n app/templates/admin_panel_users.htmlRepository: DogukanUrker/FlaskBlog
Length of output: 5791
Remove password from the user tuple passed to template — it's never used and exposes unnecessary sensitive data.
Line 46 includes u.password in the tuple passed to the template, but the template never accesses this field. Even though passwords are hashed, passing password hashes to the template violates the principle of least privilege and risks unnecessary exposure in HTML source, caches, or logs.
🔒 Proposed fix
users = [
(
u.user_id,
u.username,
u.email,
- u.password,
u.profile_picture,
u.role,
u.points,
u.time_stamp,
u.is_verified,
)
for u in users_objects
]Update the template indices: after removing u.password, the tuple indices shift by 1. In admin_panel_users.html, update:
user[4]→user[3](profile_picture)user[5]→user[4](role)user[6]→user[5](points)user[7]→user[6](timestamp)
🤖 Prompt for AI Agents
In `@app/routes/admin_panel_users.py` around lines 41 - 54, Remove the password
field from the tuple built from users_objects in the users list comprehension
(currently includes u.password) so templates do not receive password hashes;
then update the corresponding index references in admin_panel_users.html to
account for the shifted tuple positions (change user[4]→user[3],
user[5]→user[4], user[6]→user[5], user[7]→user[6]). Locate the list construction
around the users = [ (... ) for u in users_objects ] in admin_panel_users.py and
the index uses in admin_panel_users.html and adjust them accordingly.
| if not existing_user: | ||
| old_username = session["username"] | ||
|
|
||
| # Update username in users table | ||
| user = User.query.filter_by(username=old_username).first() | ||
| if user: | ||
| user.username = new_username | ||
|
|
||
| # Update author in posts table | ||
| Post.query.filter_by(author=old_username).update({"author": new_username}) | ||
|
|
||
| # Update username in comments table | ||
| Comment.query.filter_by(username=old_username).update( | ||
| {"username": new_username} | ||
| ) | ||
|
|
||
| db.session.commit() |
There was a problem hiding this comment.
Post and Comment updates execute even if the User record isn't found.
The if user: guard on Line 54 only protects the User.username assignment, but Post.query.filter_by(...) and Comment.query.filter_by(...) updates on Lines 58–63 run unconditionally, and the commit on Line 65 persists them. If the user lookup fails (defensive edge case), posts and comments get reassigned to new_username while no User record is updated.
🐛 Proposed fix — guard the entire update block
if not existing_user:
old_username = session["username"]
# Update username in users table
user = User.query.filter_by(username=old_username).first()
- if user:
- user.username = new_username
+ if not user:
+ return redirect("/")
- # Update author in posts table
- Post.query.filter_by(author=old_username).update({"author": new_username})
+ user.username = new_username
- # Update username in comments table
- Comment.query.filter_by(username=old_username).update(
- {"username": new_username}
- )
+ # Update author in posts table
+ Post.query.filter_by(author=old_username).update({"author": new_username})
- db.session.commit()
+ # Update username in comments table
+ Comment.query.filter_by(username=old_username).update(
+ {"username": new_username}
+ )
+
+ db.session.commit()🤖 Prompt for AI Agents
In `@app/routes/change_username.py` around lines 49 - 65, Post and Comment rows
(Post.query.filter_by(...) and Comment.query.filter_by(...)) and
db.session.commit() run even when the User lookup fails; move the Post and
Comment update calls and the db.session.commit() inside the same if user: guard
that wraps user.username = new_username (or replace the guard so the entire
update block is conditional on user being truthy), and add handling for the case
when User.query.filter_by(username=old_username).first() returns none (e.g.,
return/abort or skip updates) so Post, Comment and commit only execute when a
corresponding User exists.
| language = session.get("language") | ||
| translation_file = f"./translations/{language}.json" | ||
|
|
||
| with open(translation_file, "r", encoding="utf-8") as file: | ||
| translations = load(file) |
There was a problem hiding this comment.
session.get("language") can be None, leading to a FileNotFoundError.
If the session has no "language" key, language will be None, and the file path becomes ./translations/None.json, which will raise a FileNotFoundError on Line 79. Provide a default fallback.
Proposed fix
- language = session.get("language")
+ language = session.get("language", "en")📝 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.
| language = session.get("language") | |
| translation_file = f"./translations/{language}.json" | |
| with open(translation_file, "r", encoding="utf-8") as file: | |
| translations = load(file) | |
| language = session.get("language", "en") | |
| translation_file = f"./translations/{language}.json" | |
| with open(translation_file, "r", encoding="utf-8") as file: | |
| translations = load(file) |
🤖 Prompt for AI Agents
In `@app/routes/dashboard.py` around lines 76 - 80, session.get("language") can be
None causing translation_file to become "./translations/None.json" and raise
FileNotFoundError; update the code around session.get("language") and
translation_file so you use a safe default (e.g., "en") when
session.get("language") returns None or an empty value. Concretely, replace the
raw session.get("language") use with something like language =
session.get("language") or "en" (or session.get("language", "en")), then build
translation_file and proceed to open/load; ensure the change touches the
language variable, translation_file, and the with open(...) load(...) sequence
so it never attempts to open "None.json".
| post_content = request.form["post_content"] | ||
| post_abstract = request.form["post_abstract"] | ||
| post_category = request.form["post_category"] | ||
| post_banner = request.files["post_banner"].read() |
There was a problem hiding this comment.
request.files["post_banner"] can raise KeyError if the file field is absent.
If the form is submitted without a file input named post_banner, this will raise an unhandled KeyError/400. Use .get() with a fallback:
🛡️ Proposed fix
- post_banner = request.files["post_banner"].read()
+ post_banner_file = request.files.get("post_banner")
+ post_banner = post_banner_file.read() if post_banner_file else b""📝 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.
| post_banner = request.files["post_banner"].read() | |
| post_banner_file = request.files.get("post_banner") | |
| post_banner = post_banner_file.read() if post_banner_file else b"" |
🤖 Prompt for AI Agents
In `@app/routes/edit_post.py` at line 60, The current code accesses
request.files["post_banner"] directly which can raise KeyError if the field is
missing; change it to use request.files.get("post_banner") and guard before
calling .read(): retrieve the file object into a variable (e.g.,
post_banner_file = request.files.get("post_banner")), check if post_banner_file
is truthy (and optionally has a filename) and only then call
post_banner_file.read() to assign post_banner; otherwise set post_banner to None
or handle the missing-file branch (validation error/flash/redirect) inside the
same function handling the form submission.
| def login_required( | ||
| route_name: str, | ||
| redirect_to="/", | ||
| flash_page: str | None = None, | ||
| flash_message_key: str = "login", | ||
| flash_category: str = "error", | ||
| ): | ||
| """Ensure the current session has a logged in user.""" | ||
|
|
||
| def decorator(view_func): | ||
| @wraps(view_func) | ||
| def wrapped_view(*args, **kwargs): | ||
| if "username" in session: | ||
| return view_func(*args, **kwargs) | ||
|
|
||
| Log.error( | ||
| f"{request.remote_addr} tried to reach {route_name} without being logged in" | ||
| ) | ||
|
|
||
| if flash_page: | ||
| flash_message( | ||
| page=flash_page, | ||
| message=flash_message_key, | ||
| category=flash_category, | ||
| language=session.get("language", "en"), | ||
| ) | ||
|
|
||
| if callable(redirect_to): | ||
| return redirect(redirect_to(*args, **kwargs)) | ||
|
|
||
| return redirect(redirect_to) | ||
|
|
||
| return wrapped_view | ||
|
|
||
| return decorator |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which session key is used across the codebase for authentication
echo "=== Occurrences of session[\"userName\"] ==="
rg -n 'session\["userName"\]' --type=py || echo "No matches found"
echo ""
echo "=== Occurrences of session[\"username\"] ==="
rg -n 'session\["username"\]' --type=py || echo "No matches found"
echo ""
echo "=== Occurrences of session.get\(\"userName\"\) ==="
rg -n "session\.get\(\"userName\"" --type=py || echo "No matches found"
echo ""
echo "=== Occurrences of session.get\(\"username\"\) ==="
rg -n "session\.get\(\"username\"" --type=py || echo "No matches found"
echo ""
echo "=== All session key references (for context) ==="
rg -n 'session\[' --type=py | head -40Repository: DogukanUrker/FlaskBlog
Length of output: 7209
Session key usage violates coding guideline but is systemic across codebase.
The code uses session["username"] (line 54), which conflicts with the coding guideline requiring session["userName"]. However, verification reveals this discrepancy is systemic: the entire codebase (40+ locations across all authentication files) consistently uses the lowercase variant. No instances of the guideline-compliant session["userName"] exist anywhere.
The new code correctly follows the established codebase convention, but this represents a guideline-to-implementation mismatch that requires clarification from maintainers: either update the coding guideline to match actual usage, or commit to a codebase-wide migration.
The decorator design itself is sound: clean separation of concerns, proper @wraps usage for Flask endpoint registration, and sensible defaults (language fallback via session.get("language", "en")).
🤖 Prompt for AI Agents
In `@app/utils/route_guards.py` around lines 10 - 44, The decorator uses
session["username"] which conflicts with the coding guideline's
session["userName"] but the rest of the codebase consistently uses the lowercase
"username"; resolve the mismatch by choosing one canonical key and making code +
docs consistent: either (A) change this decorator to use session.get("userName")
and perform a project-wide replace of session["username"] -> session["userName"]
(update any tests and auth code that read/write the session), or (B) accept the
existing lowercase key by updating the coding guideline and any docs to declare
"username" as the canonical session key; locate the decorator function
login_required and the other auth read/write sites to apply the chosen
consistent change.
Add a comprehensive set of end-to-end tests and supporting utilities, and harden post/comment handling in the app. - app/routes/post.py: require login for POST actions, add permission checks for deleting posts/comments, validate comment existence, and log unauthorized attempts; ensure proper redirects. - tests/: add many E2E test modules (account, admin, home, post, search, pages) covering create/edit/delete posts, comments, admin flows, dashboard, static pages, and auth flows. - tests/e2e/pages/: add page objects CreatePostPage and PostPage and export them from package init. - tests/e2e/helpers/database_helpers.py: add helpers to create/query test posts and comments (create_test_post, create_test_comment, get_post_by_title/url_id, get_comment_by_id, etc.) and small utilities used by tests. - tests/e2e/helpers/__init__.py: export new helper functions. - tests/README.md: update run instructions, suite overview, fixtures, and CI notes. These changes provide test coverage and developer tooling for E2E scenarios and fix authorization/validation gaps in post/comment handling.
Capture the user's language before clearing the session to avoid reading from a cleared session in change_password route. Pass that language into the flash_message after session.clear to preserve localization. Add a new end-to-end test (tests/e2e/account/test_change_password_flow.py) that verifies credential rotation: after changing password the old password is rejected and the new password authenticates. Update tests/README.md to reflect the added test and adjust suite counts and file listings.
Standardize the change-username key/name across the app: rename "change_user_name" to "change_username" in all translation files, update the change_username template to use the new keys, adjust route flash_page and flash_message calls, and fix the form import path (utils.forms.change_username_form) in change_username route. This unifies naming for the change-username feature and fixes related template/import references.
Rename app/utils/forms/change_user_name_form.py to app/utils/forms/change_username_form.py for naming consistency. No code changes were made; update any imports or references to the old filename as needed.
DogukanUrker
force-pushed
the
feat/improve-e2e-tests
branch
from
bfb53fa to
bdd40ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@app/routes/post.py`:
- Around line 45-48: The login guard in post submission logging now interpolates
user-controlled `url_id` directly into `Log.error`, which reintroduces log
injection risk. Re-add and use `sanitize_for_log` in `app/routes/post.py` for
every externally controlled value included in the message, especially `url_id`
and `request.remote_addr`, and keep the check in the `post` route’s `if
"username" not in session:` block so the log entry cannot contain CRLF or forged
content.
In `@tests/e2e/account/test_account_settings.py`:
- Around line 59-87: The current assertion in
test_change_password_redirects_to_login is too permissive because it allows the
buggy no-op outcome as a passing result, so the test cannot catch the documented
change_password issue. Tighten the expectation in
test_change_password_redirects_to_login to require the successful redirect to
the login flow after submission, and use the existing change_password route
behavior plus _login/page interaction to validate that the password change
actually logs the user out. If the route still fails due to the
session.clear()/session["language"] ordering, fix that logic in change_password
so the redirect path becomes deterministic and the test can assert it directly.
In `@tests/README.md`:
- Around line 37-46: The local suite summary has an inconsistent test-file
count: the headline says 14 test files, but the suite table and Project
Structure tree total 13. Update the summary text in tests/README.md so the file
count matches the actual breakdown shown by the suite table, keeping the test
total and suite details aligned.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4dcdbf8-a96c-43db-9a74-ab497fa2e794
📒 Files selected for processing (49)
app/routes/account_settings.pyapp/routes/admin_panel.pyapp/routes/admin_panel_comments.pyapp/routes/admin_panel_posts.pyapp/routes/admin_panel_users.pyapp/routes/change_password.pyapp/routes/change_profile_picture.pyapp/routes/change_username.pyapp/routes/create_post.pyapp/routes/dashboard.pyapp/routes/edit_post.pyapp/routes/post.pyapp/routes/verify_user.pyapp/templates/change_username.htmlapp/translations/de.jsonapp/translations/en.jsonapp/translations/es.jsonapp/translations/fr.jsonapp/translations/hi.jsonapp/translations/ja.jsonapp/translations/pl.jsonapp/translations/pt.jsonapp/translations/ru.jsonapp/translations/tr.jsonapp/translations/uk.jsonapp/translations/zh.jsonapp/utils/forms/change_username_form.pyapp/utils/route_guards.pytests/README.mdtests/e2e/account/__init__.pytests/e2e/account/test_account_settings.pytests/e2e/account/test_change_password_flow.pytests/e2e/account/test_dashboard.pytests/e2e/account/test_profile_and_preferences.pytests/e2e/account/test_static_pages.pytests/e2e/admin/__init__.pytests/e2e/admin/test_admin.pytests/e2e/helpers/__init__.pytests/e2e/helpers/database_helpers.pytests/e2e/home/__init__.pytests/e2e/home/test_home.pytests/e2e/pages/__init__.pytests/e2e/pages/create_post_page.pytests/e2e/pages/post_page.pytests/e2e/post/__init__.pytests/e2e/post/test_post.pytests/e2e/search/__init__.pytests/e2e/search/test_category.pytests/e2e/search/test_search.py
✅ Files skipped from review due to trivial changes (7)
- tests/e2e/search/init.py
- tests/e2e/admin/init.py
- tests/e2e/post/init.py
- tests/e2e/account/init.py
- app/templates/change_username.html
- tests/e2e/home/init.py
- app/translations/pl.json
🚧 Files skipped from review as they are similar to previous changes (24)
- tests/e2e/account/test_static_pages.py
- tests/e2e/pages/init.py
- app/routes/account_settings.py
- tests/e2e/home/test_home.py
- app/routes/admin_panel_comments.py
- app/translations/de.json
- app/translations/ru.json
- app/routes/verify_user.py
- app/translations/hi.json
- tests/e2e/helpers/database_helpers.py
- tests/e2e/helpers/init.py
- app/translations/tr.json
- app/translations/es.json
- app/translations/uk.json
- app/routes/edit_post.py
- tests/e2e/pages/post_page.py
- tests/e2e/search/test_search.py
- app/routes/change_profile_picture.py
- app/routes/change_password.py
- app/routes/admin_panel_posts.py
- app/routes/admin_panel.py
- app/translations/ja.json
- app/routes/change_username.py
- app/routes/create_post.py
| if "username" not in session: | ||
| Log.error( | ||
| f'{request.remote_addr} attempted to submit to post "{url_id}" without login', | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Log injection regression: url_id is logged unsanitized.
This PR removed the sanitize_for_log import (per change details, lines 20-21), and the old per-comment login check that sanitized request.remote_addr/url_id before logging. The new guard now interpolates url_id — which is user-controlled from the URL path — directly into the log message. A crafted url_id containing CRLF can forge log entries (CWE-117).
🛡️ Proposed fix — re-sanitize logged inputs
if "username" not in session:
Log.error(
- f'{request.remote_addr} attempted to submit to post "{url_id}" without login',
+ f'{sanitize_for_log(request.remote_addr)} attempted to submit to post "{sanitize_for_log(url_id)}" without login',
)Re-add the import (around lines 20-21):
+from app.utils.log import sanitize_for_log # adjust to the actual module path📝 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.
| if "username" not in session: | |
| Log.error( | |
| f'{request.remote_addr} attempted to submit to post "{url_id}" without login', | |
| ) | |
| if "username" not in session: | |
| Log.error( | |
| f'{sanitize_for_log(request.remote_addr)} attempted to submit to post "{sanitize_for_log(url_id)}" without login', | |
| ) |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 45-47: Logging request-derived input unsanitized allows log forging (CRLF injection); strip newlines / encode the value before logging.
Context: Log.error(
f'{request.remote_addr} attempted to submit to post "{url_id}" without login',
)
Note: [CWE-117] Improper Output Neutralization for Logs. OWASP A09:2021 Security Logging and Monitoring Failures.
(log-injection-python)
🤖 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 `@app/routes/post.py` around lines 45 - 48, The login guard in post submission
logging now interpolates user-controlled `url_id` directly into `Log.error`,
which reintroduces log injection risk. Re-add and use `sanitize_for_log` in
`app/routes/post.py` for every externally controlled value included in the
message, especially `url_id` and `request.remote_addr`, and keep the check in
the `post` route’s `if "username" not in session:` block so the log entry cannot
contain CRLF or forged content.
Source: Linters/SAST tools
| def test_change_password_redirects_to_login(self, page, flask_server, test_user): | ||
| """Changing password should log out the user and redirect to login. | ||
|
|
||
| note: the change_password route has a known bug at line 76-81 where | ||
| session.clear() is called before accessing session["language"]. this | ||
| test may surface a 500 error. if it does, the route needs fixing first. | ||
| """ | ||
| _login(page, flask_server, test_user.username, test_user.password) | ||
|
|
||
| page.goto( | ||
| f"{flask_server['base_url']}/change-password", | ||
| wait_until="domcontentloaded", | ||
| ) | ||
|
|
||
| new_password = "NewPassword456!" | ||
| page.fill('input[name="old_password"]', test_user.password) | ||
| page.fill('input[name="password"]', new_password) | ||
| page.fill('input[name="password_confirm"]', new_password) | ||
| page.click('button[type="submit"]') | ||
|
|
||
| # the route should redirect to /login/ after password change. | ||
| # if the session.clear() bug is present, this may result in an error | ||
| # page instead - which is also a valid (bug-catching) outcome. | ||
| expect(page).to_have_url( | ||
| re.compile( | ||
| rf"^{re.escape(flask_server['base_url'])}/(login/.*|change-password)$" | ||
| ), | ||
| timeout=10000, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test passes regardless of the documented change_password bug, weakening coverage.
The URL assertion accepts either /login/... (success) or /change-password (the buggy/no-op outcome described in the docstring), so the test cannot fail on the very bug it references. Since test_change_password_flow.py already exercises the real rotation, consider either tightening this assertion to require the login redirect or fixing the underlying session.clear()-before-session["language"] ordering in the route. Want me to open an issue to track the route bug?
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 82-84: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(
rf"^{re.escape(flask_server['base_url'])}/(login/.*|change-password)$"
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 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 `@tests/e2e/account/test_account_settings.py` around lines 59 - 87, The current
assertion in test_change_password_redirects_to_login is too permissive because
it allows the buggy no-op outcome as a passing result, so the test cannot catch
the documented change_password issue. Tighten the expectation in
test_change_password_redirects_to_login to require the successful redirect to
the login flow after submission, and use the existing change_password route
behavior plus _login/page interaction to validate that the password change
actually logs the user out. If the route still fails due to the
session.clear()/session["language"] ordering, fix that logic in change_password
so the redirect path becomes deterministic and the test can assert it directly.
| Current local suite size: **110 tests** across **14 test files**. | ||
|
|
||
| | Suite | Files | Tests | Focus | | ||
| | ----- | ----- | ----- | ----- | | ||
| | `e2e/auth/` | 3 | 62 | Login, signup, logout, session handling | | ||
| | `e2e/account/` | 5 | 17 | Account settings, username/profile updates, password change flow, dashboard, static pages, preferences | | ||
| | `e2e/post/` | 1 | 14 | Create/edit/delete post, comments, authorization, admin moderation via protected POST flows | | ||
| | `e2e/admin/` | 1 | 8 | Admin access control, users (role + delete), comments management | | ||
| | `e2e/search/` | 2 | 6 | Search results and category filtering | | ||
| | `e2e/home/` | 1 | 3 | Home rendering and sorting routes | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test-file count is inconsistent (14 vs 13).
Line 37 states 14 test files, but the suite table sums to 13 files (3+5+1+1+2+1) and the Project Structure tree also lists 13 test_*.py files. The test count (110) matches the table; only the file count is off.
✏️ Proposed fix
-Current local suite size: **110 tests** across **14 test files**.
+Current local suite size: **110 tests** across **13 test files**.📝 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.
| Current local suite size: **110 tests** across **14 test files**. | |
| | Suite | Files | Tests | Focus | | |
| | ----- | ----- | ----- | ----- | | |
| | `e2e/auth/` | 3 | 62 | Login, signup, logout, session handling | | |
| | `e2e/account/` | 5 | 17 | Account settings, username/profile updates, password change flow, dashboard, static pages, preferences | | |
| | `e2e/post/` | 1 | 14 | Create/edit/delete post, comments, authorization, admin moderation via protected POST flows | | |
| | `e2e/admin/` | 1 | 8 | Admin access control, users (role + delete), comments management | | |
| | `e2e/search/` | 2 | 6 | Search results and category filtering | | |
| | `e2e/home/` | 1 | 3 | Home rendering and sorting routes | | |
| Current local suite size: **110 tests** across **13 test files**. |
🤖 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 `@tests/README.md` around lines 37 - 46, The local suite summary has an
inconsistent test-file count: the headline says 14 test files, but the suite
table and Project Structure tree total 13. Update the summary text in
tests/README.md so the file count matches the actual breakdown shown by the
suite table, keeping the test total and suite details aligned.
…through When delete_post or delete_comment returned False (unauthorized), the code fell through to the comment-creation path which crashed on the missing 'comment' form field, producing a 400 BAD REQUEST. Now we always redirect back to the post page regardless of success/failure.
…egular user login redirect This helps prevent flaky test failures in CI due to timing issues when the redirect resolves faster or slower than the wait listener expects. This matches the existing pattern in the admin logout test.
1 participant
fixes #222
Summary by CodeRabbit