Fix: UI improvements for email password reset pages and discussion room creation #5231
+302
−41
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
👋 Hi @e-esakman! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
github-actions
bot
added
migrations
Contributor
WalkthroughAdds a migration to rename Room.type values ("org" ↔ "organization") and constrain choices; updates ROOM_TYPES; adds two password-reset templates; replaces room creation form with a card-style form and captcha/toggle; and significantly updates chat templates/JS for escaped/linkified messages, per-message styling, reactions/emoji, and notifications. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
website/templates/account/password_reset_from_key_done.html (1)
1-32: Solid, dark-mode-friendly completion UILayout, semantics, and use of
{% trans %}for primary copy look good and consistent with the rest of the account flow. If you later expand i18n, consider wrapping the title block text in{% trans %}as well, but this is not blocking.website/templates/account/password_reset_done.html (1)
1-31: Password reset “email sent” screen looks correctThe layout and messaging are clear, accessible, and consistent with the completion template. As a minor future improvement, you could localize the
<title>text using{% trans %}, but the current implementation is fine to ship.website/templates/room_form.html (1)
85-114: Wire up the captcha reload icon to therefreshCaptchahelperYou define
refreshCaptcha(event)to refresh the captcha via AJAX, but the reload icon link currently just navigates to{% url 'room_create' %}and never calls the helper. That makes the JS dead code and forces a full page reload to get a new captcha.Hook the link to the helper while keeping the href as a graceful no-JS fallback:
- <a href="{% url 'room_create' %}" - class="text-red-500 hover:text-red-600 transition duration-200" - title="Reload captcha securely" - rel="noopener noreferrer" - aria-label="Reload captcha image safely"> + <a href="{% url 'room_create' %}" + onclick="refreshCaptcha(event)" + class="text-red-500 hover:text-red-600 transition duration-200" + title="Reload captcha securely" + rel="noopener noreferrer" + aria-label="Reload captcha image safely">The existing
refreshCaptchaimplementation below will then be exercised as intended.Also applies to: 138-152
website/templates/join_room.html (1)
24-35: UseformatMessageWithLinksinappendMessageso live messages get clickable links tooServer-rendered messages are URL-linkified (
|urlize) with good overflow handling, but WebSocket messages built inappendMessagestill render plain escaped text. This means links are only clickable for historical messages, not for new ones.You already introduced
formatMessageWithLinks(content)that safely escapes and linkifies URLs; you can reuse it here and match the classes used in the template:- messageDiv.innerHTML = ` + messageDiv.innerHTML = ` <div class="flex items-start justify-between"> <div class="flex-1"> <p class="text-sm font-medium">${escapeHtml(data.username)}</p> - <p class="text-sm text-gray-700 dark:text-gray-300">${escapeHtml(data.message)}</p> + <p class="text-sm text-gray-800 dark:text-gray-200 break-words whitespace-pre-wrap"> + ${formatMessageWithLinks(data.message)} + </p> <div class="flex items-center gap-2 mt-1"> <span class="text-xs text-gray-500 dark:text-gray-500">just now</span> <div class="reactions-container flex items-center gap-1 min-h-[28px]"></div>This keeps usernames escaped, gives live messages the same link behavior as initial messages, and maintains dark-mode-friendly styling.
Also applies to: 468-535
website/templates/rooms_list.html (1)
63-80: Secure linkification for quick-chat messages looks good (optional robustness tweak)The combination of
escapeHtmlandformatMessageWithLinksgives you clickable URLs in quick-chat while still escaping user content, which is the right tradeoff for XSS safety.If you want to harden it slightly against unexpected
null/non-stringcontentvalues from the API, you can coerce before escaping:- function formatMessageWithLinks(content) { - let safeContent = escapeHtml(content); + function formatMessageWithLinks(content) { + if (typeof content !== 'string') { + content = content == null ? '' : String(content); + } + let safeContent = escapeHtml(content);Not required for correctness, but it makes the helper more defensive.
Also applies to: 488-545
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
website/migrations/0261_alter_room_type.py(1 hunks)website/models.py(1 hunks)website/templates/account/password_reset_done.html(1 hunks)website/templates/account/password_reset_from_key_done.html(1 hunks)website/templates/join_room.html(6 hunks)website/templates/room_form.html(1 hunks)website/templates/rooms_list.html(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
website/models.py (1)
2063-2072: ROOM_TYPES change looks consistent with the new migrationUpdating the key from
"org"to"organization"aligns the model with the newCharFieldchoices and the updated templates; the enum surface is consistent and clear. No model-level issues here; data-migration concerns are noted alongside the migration itself.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
Update room type from 'org' to 'organization' and vice versa.
github-actions
bot
added
the
changes-requested
Contributor
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
github-actions
bot
added
pre-commit: failed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/migrations/0261_alter_room_type.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/migrations/0261_alter_room_type.py (1)
website/models.py (1)
Room(2062-2086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest outputFor more information, see the Django testing documentation. |
github-actions
bot
added
tests: failed
Contributor
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest outputFor more information, see the Django testing documentation. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
website/migrations/0261_alter_room_type.py (2)
28-44: Drop the secondRunPythonoperation; one is sufficient.The data migration
RunPython(forwards, backwards)appears before and after theAlterField. This causes the same data update to run twice on migrate and twice on rollback, which is redundant and may confuse future maintainers.You only need it once, before the schema change:
operations = [ migrations.RunPython(forwards, backwards), migrations.AlterField( model_name="room", name="type", field=models.CharField( choices=[ ("project", "Project"), ("bug", "Bug"), ("organization", "Organization"), ("custom", "Custom"), ], max_length=20, ), ), - migrations.RunPython(forwards, backwards), ]
3-3: Importmodelsto avoidNameErrorin migration.
models.CharFieldis used in theAlterFieldbutmodelsis not imported, so running this migration will raise aNameError.Apply this diff:
-from django.db import migrations +from django.db import migrations, models
🧹 Nitpick comments (1)
website/migrations/0261_alter_room_type.py (1)
5-21: Remove duplicateforwards/backwardsdefinitions.
forwardsandbackwardsare defined twice with identical bodies. Python will keep only the second pair, and the first becomes dead code, which is confusing and may trip linters.Consider keeping a single pair (e.g., the first) and deleting the duplicate:
def forwards(apps, schema_editor): Room = apps.get_model("website", "Room") Room.objects.filter(type="org").update(type="organization") def backwards(apps, schema_editor): Room = apps.get_model("website", "Room") Room.objects.filter(type="organization").update(type="org") - -def forwards(apps, schema_editor): - Room = apps.get_model("website", "Room") - Room.objects.filter(type="org").update(type="organization") - - -def backwards(apps, schema_editor): - Room = apps.get_model("website", "Room") - Room.objects.filter(type="organization").update(type="org") -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
website/migrations/0261_alter_room_type.py(1 hunks)website/templates/account/password_reset_done.html(1 hunks)website/templates/account/password_reset_from_key_done.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/templates/account/password_reset_done.html
- website/templates/account/password_reset_from_key_done.html
🧰 Additional context used
🧬 Code graph analysis (1)
website/migrations/0261_alter_room_type.py (1)
website/models.py (1)
Room(2062-2086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
Contributor
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
Contributor
|
@e-esakman I was thinking of taking up this issue. Nice Work! |
arnavkirti
suggested changes
Dec 9, 2025
Contributor
arnavkirti
left a comment
arnavkirti
left a comment
There was a problem hiding this comment.
Please correct the critical issues that coderabbitai suggested. Thanks!!
Jayant2908
approved these changes
Dec 9, 2025
Contributor
There was a problem hiding this comment.
Fix the coderabbitai error and you're good to go, LGTM! Just a few small non-blocker though:-
1)Room listing performance – join_room.html still renders room_messages directly, querying room.screenshots and room.tags in the template. but if these rooms get busy, a small prefetch_related("screenshots", "tags") on the queryset could help avoid N+1 queries.
2)Password reset templates & i18n – The new password reset templates look much cleaner. Most strings are wrapped in {% trans %}, but a couple of tagline/marketing lines are still plain text; if full localization is a goal, those might eventually want translation tags as well.
github-actions
bot
added
changes-requested
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
website/templates/account/password_reset_from_key_done.html (1)
11-19: Consider i18n and heading wording polish for this success screenThe template works as-is, but two small UX/i18n tweaks are worth considering:
- Wrap the BLT marketing paragraph and “Join our community” CTA in
{% blocktrans %}/{% trans %}to keep them localizable.- The right‑side heading says “Change Password” even though the page is a “success” state; something like “Password Successfully Changed” would better match the title and alert copy.
These are non‑blocking, just polish.
website/templates/account/password_reset_done.html (1)
11-19: Optional: localize shared marketing block for consistency with other textEverything here is structurally correct. To keep i18n consistent with the rest of the template (and with
password_reset_from_key_done.html), consider wrapping the BLT description paragraph and “Join our community” link text in{% blocktrans %}/{% trans %}so they’re also translatable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
website/templates/account/password_reset_done.html(1 hunks)website/templates/account/password_reset_from_key_done.html(1 hunks)website/tests/test_rooms.py(1 hunks)website/views/organization.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
website/tests/test_rooms.py (1)
website/models.py (1)
Room(2062-2086)
website/views/organization.py (1)
website/models.py (1)
Room(2062-2086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
website/tests/test_rooms.py (1)
16-18: Test Room now correctly sets a validtypechoiceUsing
type="project"here aligns the test with theRoom.ROOM_TYPESchoices and avoids failures after the migration madetyperequired. No further changes needed.
arnavkirti
approved these changes
Dec 9, 2025
github-actions
bot
added
files-changed: 8
Contributor
Author
|
@DonnieBLT sir, it has passed all the tests. It's ready for final review and merge. Please let me know , if any change required. |
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
github-actions
bot
added
tests: failed
github-actions
bot
added
tests: passed
Contributor
Author
|
@DonnieBLT sir , if u could, please review it. If any changes, let me know. |
github-actions
bot
added
last-active: 1d
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #5214
Issues Fixed:
Changes in discussion room before
Before
before.mp4
After
Untitled.design.mp4
Email change
Before
After
Migration Note:
A schema migration (0261_alter_room_type.py) was added to fix a mismatch in the Room model’s type field choices.
The field originally used "org", but during room creation "organization" was being passed , causing the error:
“Select a valid choice. 'organization' is not one of the available choices.”
The migration ensures both the code and database use "organization" consistently.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.