Story 2412: Sign Up Page Integration#2482
Open
jlchilders11 wants to merge 10 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR centralizes V3 auth-page context into a mixin, requires terms-of-use on signup, refactors the signup template to render social providers dynamically, updates CSS to disable social actions until ToU is accepted, and removes the old V3 signup URL. ChangesV3 Signup Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
users/views.py (1)
551-565:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hard-gate the live signup view with
V3AuthContextMixin.Because this mixin returns 404 when the
v3flag is off,/accounts/signup/no longer falls back to the legacy signup flow at all. That conflicts with the PR goal of swapping designs behind the flag on the existing route. Use a non-gating mixin here, or make the 404 behavior opt-in for V3-only pages.🤖 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 `@users/views.py` around lines 551 - 565, The class CustomSignupView must not inherit V3AuthContextMixin because that mixin currently returns 404 when the v3 flag is off and thereby blocks the legacy signup route; remove V3AuthContextMixin from the inheritance list on CustomSignupView and instead apply its behavior conditionally: keep ClaimExistingAccountMixin and SignupView in the bases, and override get_context_data (or dispatch) to call V3AuthContextMixin.get_context_data(self, **kwargs) only when flag_is_active(self.request, "v3") is true so the view falls back to the legacy flow when the flag is off; reference the class name CustomSignupView and the mixin V3AuthContextMixin when making this change.
🤖 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 `@core/mixins.py`:
- Around line 25-33: The dispatch method currently clears self._v3_active for
any non-GET, causing POSTs that re-render to lose the v3 template; update
dispatch so self._v3_active is set to True whenever self.v3_template_name and
flag_is_active(request, "v3") (regardless of request.method), and only return
self.render_v3_response immediately on GET; remove the unconditional
self._v3_active = False so the flag remains True for POST re-renders (symbols:
dispatch, _v3_active, v3_template_name, flag_is_active, render_v3_response,
request.method).
- Around line 91-93: The reverse_lazy call in core/mixins.py sets
context["signup_url"] = reverse_lazy("v3-signup") but config/v3_urls.py does not
define a pattern named "v3-signup", causing a possible NoReverseMatch; fix by
either updating the mixin to use the actual route name defined in
config/v3_urls.py (replace "v3-signup" with the real name used there) or
add/restore a URL pattern in config/v3_urls.py with name="v3-signup" that points
to the signup view so reverse_lazy("v3-signup") resolves correctly; locate the
assignment to context["signup_url"] in the mixin to apply the change or add the
named URL pattern in config/v3_urls.py.
In `@static/css/v3/auth-page.css`:
- Around line 188-191: The CSS selector never matches because the template uses
inputs with class .auth-page__invisible-check (no name) for social checkboxes;
update the rule to target that class instead of input[name="checkbox-GitHub"].
Specifically, change the :has(...) portion to look for
.auth-page__invisible-check:checked (e.g.
.auth-page__signup-form:not(:has(input[name="accept_terms_of_use"]:checked)):has(.auth-page__invisible-check:checked)
input[name="accept_terms_of_use"]) so the TOU highlight applies when the social
checkbox is checked.
- Line 200: Replace the deprecated CSS property clip: rect(0, 0, 0, 0) used for
visually-hidden inputs with the modern equivalent clip-path: inset(50%) in the
selectors that implement the visually-hidden pattern (e.g.,
.auth-page__invisible-check and .checkbox__input); locate those selectors and
swap the clip declaration for clip-path: inset(50%) (keeping the rest of the
visually-hidden rules intact such as position, width/height, overflow,
white-space, and border: 0).
In `@templates/v3/accounts/signup.html`:
- Around line 57-70: Render the server-side validation errors for the checkbox
field named "accept_terms_of_use" and correct the label copy to "Terms of Use":
update the template around the input with id "checkbox-accept_terms_of_use" to
output the field's errors (e.g., render {{ form.accept_terms_of_use.errors }} or
use the form field's non_field_errors helper) and associate them with the input
(add an error container and aria-describedby if needed), and change the link
text inside the span with class "checkbox__label" from "Term of Use" to "Terms
of Use".
- Line 30: Replace the hardcoded form action and links that point to
"/accounts/signup/" with Django URL reversal using the named URL account_signup;
update the form tag in templates/v3/accounts/signup.html to use {% url
'account_signup' %} for the action and likewise replace any <a
href="/accounts/signup/"> occurrences (for example in
templates/users/includes/user_join.html) with {% url 'account_signup' %} so
paths follow the named route and respect any URL prefix changes.
---
Outside diff comments:
In `@users/views.py`:
- Around line 551-565: The class CustomSignupView must not inherit
V3AuthContextMixin because that mixin currently returns 404 when the v3 flag is
off and thereby blocks the legacy signup route; remove V3AuthContextMixin from
the inheritance list on CustomSignupView and instead apply its behavior
conditionally: keep ClaimExistingAccountMixin and SignupView in the bases, and
override get_context_data (or dispatch) to call
V3AuthContextMixin.get_context_data(self, **kwargs) only when
flag_is_active(self.request, "v3") is true so the view falls back to the legacy
flow when the flag is off; reference the class name CustomSignupView and the
mixin V3AuthContextMixin when making this change.
🪄 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 Plus
Run ID: 3d996dd4-cad4-4073-87f7-372795a19ae2
📒 Files selected for processing (7)
config/v3_urls.pycore/mixins.pystatic/css/v3/auth-page.cssstatic/css/v3/forms.csstemplates/v3/accounts/signup.htmlusers/forms.pyusers/views.py
💤 Files with no reviewable changes (1)
- config/v3_urls.py
| padding: 0; | ||
| margin: -1px; | ||
| overflow: hidden; | ||
| clip: rect(0, 0, 0, 0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify deprecated clip usage locations in v3 CSS.
rg -n 'clip:\s*rect\(' static/css/v3/auth-page.css static/css/v3/forms.cssRepository: boostorg/website-v2
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,215p' static/css/v3/auth-page.css
echo '---'
sed -n '565,620p' static/css/v3/forms.cssRepository: boostorg/website-v2
Length of output: 2188
Replace deprecated clip in visually-hidden inputs.
clip: rect(0, 0, 0, 0); is used for visually-hidden elements in static/css/v3/auth-page.css (.auth-page__invisible-check, ~200) and static/css/v3/forms.css (.checkbox__input, ~593). Replace it with clip-path: inset(50%) to satisfy stylelint and modernize the pattern.
Suggested fix
- clip: rect(0, 0, 0, 0);
+ clip-path: inset(50%);📝 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.
Suggested change
| clip: rect(0, 0, 0, 0); | |
| clip-path: inset(50%); |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 200-200: Deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
🤖 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 `@static/css/v3/auth-page.css` at line 200, Replace the deprecated CSS property
clip: rect(0, 0, 0, 0) used for visually-hidden inputs with the modern
equivalent clip-path: inset(50%) in the selectors that implement the
visually-hidden pattern (e.g., .auth-page__invisible-check and
.checkbox__input); locate those selectors and swap the clip declaration for
clip-path: inset(50%) (keeping the rest of the visually-hidden rules intact such
as position, width/height, overflow, white-space, and border: 0).
Source: Linters/SAST tools
| id="signup_form" | ||
| method="post" | ||
| action="" | ||
| action="/accounts/signup/" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the project has a named signup URL and detect hardcoded signup actions.
fd -i 'urls.py' | xargs rg -n "account_signup|accounts/signup|name=['\"]account_signup['\"]"
rg -n 'action="/accounts/signup/"' templatesRepository: boostorg/website-v2
Length of output: 521
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect surrounding lines in the template
echo "---- templates/v3/accounts/signup.html:25-40 ----"
nl -ba templates/v3/accounts/signup.html | sed -n '25,45p'
# 2) Find all hardcoded signup action usages
echo "---- hardcoded action=\"/accounts/signup/\" in templates ----"
rg -n 'action="/accounts/signup/"' templates || true
# 3) Confirm how account_signup is defined and referenced
echo "---- definition of name=\"account_signup\" ----"
rg -n 'name="account_signup"' config/urls.py
echo "---- usages of account_signup (templates) ----"
rg -n "{%\s*url\s+['\"]account_signup['\"]\s*%}" -S templates || true
echo "---- usages of /accounts/signup/ in templates ----"
rg -n '/accounts/signup/' -S templates || true
# 4) Check tests expectations
echo "---- tests referencing account_signup ----"
rg -n 'account_signup|/accounts/signup/' users/tests/test_urls.pyRepository: boostorg/website-v2
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- templates/v3/accounts/signup.html:25-40 ----"
cat -n templates/v3/accounts/signup.html | sed -n '25,45p'
echo "---- hardcoded action=\"/accounts/signup/\" in templates ----"
rg -n 'action="/accounts/signup/"' templates || true
echo "---- definition of name=\"account_signup\" ----"
rg -n 'name="account_signup"' config/urls.py
echo "---- usages of account_signup via {% url %} (templates) ----"
rg -n "{%[[:space:]]*url[[:space:]]+['\"]account_signup['\"][[:space:]]*%}" -S templates || true
echo "---- usages of /accounts/signup/ (templates) ----"
rg -n '/accounts/signup/' -S templates || true
echo "---- tests referencing account_signup and /accounts/signup/ ----"
rg -n 'account_signup|/accounts/signup/' users/tests/test_urls.pyRepository: boostorg/website-v2
Length of output: 1987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- config/urls.py:120-160 ----"
sed -n '120,160p' config/urls.py | cat -n
echo "---- any existing {% url 'account_signup' %} usage in templates ----"
rg -n "{%\s*url\s+['\"]account_signup['\"]\s*%}" -S templates || true
echo "---- any existing {% url account_signup %} usage (non-quoted) in templates ----"
rg -n "{%\s*url\s+account_signup\s*%}" -S templates || true
echo "---- all hardcoded /accounts/signup/ occurrences in templates ----"
rg -n '/accounts/signup/' templatesRepository: boostorg/website-v2
Length of output: 2569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- templates/users/includes/user_join.html ----"
cat -n templates/users/includes/user_join.html | sed -n '1,80p'
echo "---- any 'account_signup' string in templates ----"
rg -n "account_signup" templates || true
echo "---- any url tag usage referencing account_* in templates ----"
rg -n "{%[[:space:]]*url" templates | head -n 50
echo "---- all hardcoded /accounts/signup/ occurrences in repo (limit to html/templates) ----"
rg -n '/accounts/signup/' templates
echo "---- all hardcoded /accounts/signup/ occurrences in entire repo (sanity) ----"
rg -n '/accounts/signup/' . | head -n 50Repository: boostorg/website-v2
Length of output: 843
Use URL reversal for the signup form action (and remove hardcoded signup URLs)
templates/v3/accounts/signup.html hardcodes action="/accounts/signup/", but account_signup is a named URL; use {% url 'account_signup' %} to avoid path/prefix fragility.
Suggested fix
- action="/accounts/signup/"
+ action="{% url 'account_signup' %}"Also consider replacing the hardcoded <a href="/accounts/signup/"> in templates/users/includes/user_join.html with {% url 'account_signup' %}.
📝 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.
Suggested change
| action="/accounts/signup/" | |
| action="{% url 'account_signup' %}" |
🤖 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 `@templates/v3/accounts/signup.html` at line 30, Replace the hardcoded form
action and links that point to "/accounts/signup/" with Django URL reversal
using the named URL account_signup; update the form tag in
templates/v3/accounts/signup.html to use {% url 'account_signup' %} for the
action and likewise replace any <a href="/accounts/signup/"> occurrences (for
example in templates/users/includes/user_join.html) with {% url 'account_signup'
%} so paths follow the named route and respect any URL prefix changes.
Comment on lines
+57
to
+70
| <label class="checkbox" for="checkbox-accept_terms_of_use"> | ||
| <input | ||
| class="checkbox__input" | ||
| type="checkbox" | ||
| id="checkbox-accept_terms_of_use" | ||
| name="accept_terms_of_use" | ||
| value="on" | ||
| required | ||
| > | ||
| <span class="checkbox__box" aria-hidden="true"> | ||
| {% include "includes/icon.html" with icon_name="check" icon_class="checkbox__check" %} | ||
| </span> | ||
| <span class="checkbox__label">I have read and accepted the <a href="{% url 'terms-of-use' %}">Term of Use</a> for this service</span> | ||
| </label> |
There was a problem hiding this comment.
Render terms-field errors and fix the label copy.
The checkbox is validated server-side, but its error is not displayed in this template. Also, “Term of Use” should be “Terms of Use”.
Suggested fix
<label class="checkbox" for="checkbox-accept_terms_of_use">
<input
class="checkbox__input"
type="checkbox"
id="checkbox-accept_terms_of_use"
name="accept_terms_of_use"
value="on"
+ {% if form.accept_terms_of_use.value %}checked{% endif %}
required
>
@@
- <span class="checkbox__label">I have read and accepted the <a href="{% url 'terms-of-use' %}">Term of Use</a> for this service</span>
+ <span class="checkbox__label">I have read and accepted the <a href="{% url 'terms-of-use' %}">Terms of Use</a> for this service</span>
</label>
+ {% if form.accept_terms_of_use.errors %}
+ {% for error in form.accept_terms_of_use.errors %}<p class="field__error" role="alert">{{ error }}</p>{% endfor %}
+ {% endif %}🤖 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 `@templates/v3/accounts/signup.html` around lines 57 - 70, Render the
server-side validation errors for the checkbox field named "accept_terms_of_use"
and correct the label copy to "Terms of Use": update the template around the
input with id "checkbox-accept_terms_of_use" to output the field's errors (e.g.,
render {{ form.accept_terms_of_use.errors }} or use the form field's
non_field_errors helper) and associate them with the input (add an error
container and aria-describedby if needed), and change the link text inside the
span with class "checkbox__label" from "Term of Use" to "Terms of Use".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
templates/v3/accounts/signup.html (1)
84-92: ⚡ Quick winDon’t silently drop non-GitHub/Google providers in the dynamic loop.
Line 84 iterates all providers, but Lines 88–92 only render buttons for GitHub/Google. Any other configured provider gets no visible action. Add an
elsefallback button (generic icon/text) so available providers are consistently surfaced.🤖 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 `@templates/v3/accounts/signup.html` around lines 84 - 92, The loop over socialaccount_providers currently only renders buttons for provider.name == "GitHub" and "Google", silently dropping other providers; update the template around the {% for provider in socialaccount_providers %} block (the code that uses {% provider_login_url provider process="login" ... as href %} and the checkbox id="checkbox-{{provider.name}}") to add an else fallback branch that includes "v3/includes/_button.html" with a generic label like "Continue with " + provider.name, the href from the provider_login_url tag, and a generic icon_name/style so any non-GitHub/Google provider will render a visible button instead of being omitted.
🤖 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.
Nitpick comments:
In `@templates/v3/accounts/signup.html`:
- Around line 84-92: The loop over socialaccount_providers currently only
renders buttons for provider.name == "GitHub" and "Google", silently dropping
other providers; update the template around the {% for provider in
socialaccount_providers %} block (the code that uses {% provider_login_url
provider process="login" ... as href %} and the checkbox
id="checkbox-{{provider.name}}") to add an else fallback branch that includes
"v3/includes/_button.html" with a generic label like "Continue with " +
provider.name, the href from the provider_login_url tag, and a generic
icon_name/style so any non-GitHub/Google provider will render a visible button
instead of being omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dcda7009-7065-45fc-a0fe-9103de6f596d
📒 Files selected for processing (2)
static/css/v3/auth-page.csstemplates/v3/accounts/signup.html
🚧 Files skipped from review as they are similar to previous changes (1)
- static/css/v3/auth-page.css
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@static/css/v3/auth-page.css`:
- Around line 157-173: The CSS has a typo in the custom property name inside the
.auth-page__sign-in-link rule: replace the incorrect --letter-spaceing-tight
with the correct --letter-spacing-tight so the class uses the defined variable;
update the declaration in the .auth-page__sign-in-link selector to reference
--letter-spacing-tight.
- Around line 187-203: The CSS here has a misspelled custom property name in the
.auth-page__signup-divider rule (uses --letter-spaceing-tight); update that
property to the correct --letter-spacing-tight so the font-letter spacing uses
the intended variable, and ensure any other occurrences in that selector (and
its descendants like .auth-page__signup-divider hr) are corrected to the exact
symbol --letter-spacing-tight.
🪄 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 Plus
Run ID: 483f23a7-cd94-4477-9a01-b8939c56d767
📒 Files selected for processing (3)
core/mixins.pystatic/css/v3/auth-page.csstemplates/v3/accounts/signup.html
8 tasks
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@core/mixins.py`:
- Around line 26-29: The current V3Mixin.dispatch sets self._v3_active and
immediately returns super().dispatch, which skips any render_v3_response()
overrides; change dispatch so it still sets self._v3_active = True, then call
response = super().dispatch(request, *args, **kwargs), then invoke
self.render_v3_response(request, response) (or call it only if it exists/if
_v3_active) so view-specific v3 behavior (redirects/cookies/context) runs, and
finally return the (possibly modified) response; update V3Mixin.dispatch to use
this flow and reference self._v3_active, render_v3_response, and dispatch when
locating the code to change.
In `@core/views.py`:
- Around line 116-119: The current CalendarView.get_context_data replaces the
V3Mixin.get_context_data by starting from {}, so
CalendarView.get_v3_context_data and shared v3 keys (like timezone) are never
merged; change get_context_data to first call super().get_context_data(**kwargs)
(invoking V3Mixin.get_context_data), then set ctx["boost_calendar"] =
settings.BOOST_CALENDAR and return ctx so v3/calendar.html receives the merged
v3 context along with the boost_calendar flag.
In `@libraries/views.py`:
- Around line 127-131: The get_v3_context_data method is re-calling
get_queryset() and can duplicate work and warnings; instead use the resolved
list passed in or the already-populated self.object_list. Change
get_v3_context_data to avoid calling get_queryset() again — prefer the incoming
queryset parameter if not None, else use self.object_list — and pass that value
to super().get_v3_context_data as object_list/queryset so you don't re-run
get_queryset() or replay messages.add_message paths.
In `@news/views.py`:
- Around line 179-183: Replace the extra database call in get_context_data by
using the already-evaluated list stored on the view: stop calling
self.get_queryset() and use self.object_list as the source for
pagination/context. Update the get_context_data implementation (method
get_context_data on the ListView subclass) to pass queryset=self.object_list and
object_list=self.object_list into super().get_context_data(...) so you don't
re-run the queryset or the display_publish_at loop.
🪄 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 Plus
Run ID: b5776217-96a2-46fa-80a6-ded1d21e62cd
📒 Files selected for processing (7)
ak/views.pycore/mixins.pycore/views.pylibraries/views.pynews/views.pyusers/views.pyversions/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- users/views.py
Comment on lines
26
to
+29
| def dispatch(self, request, *args, **kwargs): | ||
| if self.v3_template_name and flag_is_active(request, "v3"): | ||
| self._v3_active = True | ||
| return self.render_v3_response() | ||
| return super().dispatch(request, *args, **kwargs) |
There was a problem hiding this comment.
This bypasses existing render_v3_response() overrides.
Several supplied views still put real behavior in render_v3_response() instead of only template selection — see core/views.py Line 135, news/views.py Line 98, and libraries/views.py Line 462. After routing v3 requests straight through super().dispatch(...), those code paths never run, so v3 requests silently lose their redirect/cookie/context setup. Either keep the hook in V3Mixin.dispatch() or migrate the remaining overrides in the same PR.
🤖 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 `@core/mixins.py` around lines 26 - 29, The current V3Mixin.dispatch sets
self._v3_active and immediately returns super().dispatch, which skips any
render_v3_response() overrides; change dispatch so it still sets self._v3_active
= True, then call response = super().dispatch(request, *args, **kwargs), then
invoke self.render_v3_response(request, response) (or call it only if it
exists/if _v3_active) so view-specific v3 behavior (redirects/cookies/context)
runs, and finally return the (possibly modified) response; update
V3Mixin.dispatch to use this flow and reference self._v3_active,
render_v3_response, and dispatch when locating the code to change.
Comment on lines
116
to
119
| def get_context_data(self, **kwargs): | ||
| ctx = super().get_context_data(**kwargs) | ||
| ctx = {} | ||
| ctx["boost_calendar"] = settings.BOOST_CALENDAR | ||
| return ctx |
There was a problem hiding this comment.
Don't bypass V3Mixin.get_context_data() here.
Starting from {} means the v3 path never merges get_v3_context_data(), so CalendarView.get_v3_context_data() is effectively dead and v3/calendar.html loses timezone and any future shared v3 keys. Keep the super().get_context_data(**kwargs) call and then add boost_calendar.
Suggested fix
def get_context_data(self, **kwargs):
- ctx = {}
+ ctx = super().get_context_data(**kwargs)
ctx["boost_calendar"] = settings.BOOST_CALENDAR
return ctx📝 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.
Suggested change
| def get_context_data(self, **kwargs): | |
| ctx = super().get_context_data(**kwargs) | |
| ctx = {} | |
| ctx["boost_calendar"] = settings.BOOST_CALENDAR | |
| return ctx | |
| def get_context_data(self, **kwargs): | |
| ctx = super().get_context_data(**kwargs) | |
| ctx["boost_calendar"] = settings.BOOST_CALENDAR | |
| return ctx |
🤖 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 `@core/views.py` around lines 116 - 119, The current
CalendarView.get_context_data replaces the V3Mixin.get_context_data by starting
from {}, so CalendarView.get_v3_context_data and shared v3 keys (like timezone)
are never merged; change get_context_data to first call
super().get_context_data(**kwargs) (invoking V3Mixin.get_context_data), then set
ctx["boost_calendar"] = settings.BOOST_CALENDAR and return ctx so
v3/calendar.html receives the merged v3 context along with the boost_calendar
flag.
Comment on lines
127
to
+131
| def get_v3_context_data(self, queryset=None, **kwargs): | ||
| context = {} | ||
| queryset = self.get_queryset() | ||
| context = super().get_v3_context_data( | ||
| **kwargs, object_list=queryset, queryset=queryset | ||
| ) |
There was a problem hiding this comment.
Reuse the resolved list queryset instead of calling get_queryset() again.
ListView.get() has already populated self.object_list before context building. Re-running get_queryset() here does the same work twice, and in this class it also replays the messages.add_message(...) path when no release data exists, which can duplicate the warning on a single request. Prefer the incoming queryset or self.object_list.
Suggested fix
def get_v3_context_data(self, queryset=None, **kwargs):
- queryset = self.get_queryset()
+ queryset = queryset if queryset is not None else self.object_list
context = super().get_v3_context_data(
**kwargs, object_list=queryset, queryset=queryset
)📝 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.
Suggested change
| def get_v3_context_data(self, queryset=None, **kwargs): | |
| context = {} | |
| queryset = self.get_queryset() | |
| context = super().get_v3_context_data( | |
| **kwargs, object_list=queryset, queryset=queryset | |
| ) | |
| def get_v3_context_data(self, queryset=None, **kwargs): | |
| queryset = queryset if queryset is not None else self.object_list | |
| context = super().get_v3_context_data( | |
| **kwargs, object_list=queryset, queryset=queryset | |
| ) |
🤖 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 `@libraries/views.py` around lines 127 - 131, The get_v3_context_data method is
re-calling get_queryset() and can duplicate work and warnings; instead use the
resolved list passed in or the already-populated self.object_list. Change
get_v3_context_data to avoid calling get_queryset() again — prefer the incoming
queryset parameter if not None, else use self.object_list — and pass that value
to super().get_v3_context_data as object_list/queryset so you don't re-run
get_queryset() or replay messages.add_message paths.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Issue: #2412
Summary & Context
Integrates the existing sign up flow with v3 design.
Changes
Please list any potential risks or areas that need extra attention during review/testing
TESTING CONSIDERATIONS: When testing, the V3 flag must be set to "Everyone: Yes" in the admin, since the sign up page does not check the users flag. Additionally, Oauth only populates on the sing up page if the keys are set for local development. Contact @jlchilders11 for these keys if needed during testing. Finally, the Oauth workflow can only be completed on a live site, such as staging.
Additionally, the Email Sent and Email Confirmed designs are still based on V2. An additional ticket is being created to address this UI requirement.
Screenshots
Full Sign Up
Sign Up without Oauth Set:
Current Confirmation Sent Page:
Current Confirm Email Page:
New Sign up:
Error State:
Terms Accepted:
Self-review Checklist
Summary by CodeRabbit
New Features
Style
Refactor