Conversation
|
@rhodges @pollardld I added you two on this PR for visibility, but I feel ok enough about this change that your review is not required. |
There was a problem hiding this comment.
Pull request overview
Adds a user-facing “Change Password” flow using a Bootstrap modal + AJAX, backed by a Django PasswordChangeView, and removes legacy/unused login/registration templates.
Changes:
- Add
TEKDBPasswordChangeViewendpoint (/change_password/) returning JSON for success/errors. - Add “Change Password” modal + client-side AJAX submission/error rendering.
- Remove unused login/registration/forgot templates and dead view code; add login view tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| TEKDB/login/views.py | Introduces JSON-based password-change view and removes unused forgot view code. |
| TEKDB/login/tests/test_views.py | Adds tests covering login endpoints and password-change view helpers. |
| TEKDB/login/templates/registration/registration_form.html | Removes legacy registration template. |
| TEKDB/login/templates/registration/login.html | Removes dead/commented markup. |
| TEKDB/login/templates/forgot.html | Removes unused forgot-password template. |
| TEKDB/login/templates/create.html | Removes unused template. |
| TEKDB/login/templates/change_password.html | Adds the password change form markup used in the modal. |
| TEKDB/login/static/login/js/account.js | Adds AJAX helper for password changes. |
| TEKDB/explore/templates/navbar.html | Fixes href typo and adds “Change Password” menu item. |
| TEKDB/explore/templates/modals.html | Adds “Change Password” modal content and updates modal close buttons. |
| TEKDB/explore/static/explore/js/modals.js | Wires up modal submit handling, success message, and error rendering. |
| TEKDB/explore/static/explore/css/forms.css | Removes registration helptext CSS rule. |
| TEKDB/TEKDB/urls.py | Adds /change_password/ route. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return JsonResponse(context) | ||
|
|
||
|
|
||
| class TEKDBPasswordChangeView(PasswordChangeView): |
There was a problem hiding this comment.
TEKDBPasswordChangeView inherits PasswordChangeView but doesn't set template_name and the repo doesn't appear to have Django's default registration/password_change_form.html. A GET to /change_password/ will likely raise TemplateDoesNotExist. Either provide/point to a template, or explicitly disallow GET (e.g., only allow POST and return 405 for others).
| class TEKDBPasswordChangeView(PasswordChangeView): | |
| class TEKDBPasswordChangeView(PasswordChangeView): | |
| http_method_names = ["post"] |
There was a problem hiding this comment.
We are using a modal so we don't need a template file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }, | ||
| error: function(response) { | ||
| console.log('%cerror with change password request submission: %o', 'color: red', response.data); |
There was a problem hiding this comment.
In the AJAX error callback, response.data is not a standard jQuery jqXHR field (it will typically be undefined). This makes the console message misleading and can hide the actual server response. Log response.responseJSON / response.responseText instead.
| console.log('%cerror with change password request submission: %o', 'color: red', response.data); | |
| console.log('%cerror with change password request submission: %o', 'color: red', response.responseJSON || response.responseText); |
| return render(request, "forgot.html", context) | ||
| # return HttpResponse("<h1>Forgot Password") | ||
|
|
||
|
|
There was a problem hiding this comment.
This PR removes the forgot (and related templates) from login.views, but TEKDB/login/urls.py still defines routes pointing at views.forgot and views.create. If login.urls is ever re-included, importing it will crash due to missing view attributes. Consider removing or updating those stale URL patterns as part of this cleanup.
| def forgot(request): | |
| return JsonResponse( | |
| { | |
| "success": False, | |
| "error": "This endpoint is no longer available.", | |
| }, | |
| status=410, | |
| ) | |
| def create(request): | |
| return JsonResponse( | |
| { | |
| "success": False, | |
| "error": "This endpoint is no longer available.", | |
| }, | |
| status=410, | |
| ) |
| class TEKDBPasswordChangeView(PasswordChangeView): | ||
| def form_invalid(self, form): | ||
| return JsonResponse({"data": form.errors, "success": False}, status=400) | ||
|
|
||
| def form_valid(self, form): | ||
| self.object = form.save() | ||
| # prevent user’s auth session to be invalidated | ||
| # and user have to log in again after password change | ||
| update_session_auth_hash(self.request, self.object) | ||
| return JsonResponse({"data": None, "success": True}, status=200) |
There was a problem hiding this comment.
TEKDBPasswordChangeView is mounted at /change_password/ and still supports GET via PasswordChangeView. Since the project doesn’t have registration/password_change_form.html, a GET request will raise TemplateDoesNotExist (500). If this endpoint is intended to be AJAX-only, explicitly disallow GET (e.g., http_method_names = ['post'] or implement get() to return 405). Alternatively, set template_name to an existing template if GET should render a page.
| $("#changePasswordModal").on("shown.bs.modal", function () { | ||
| const changePasswordForm = document.querySelector("#changePasswordModal form"); | ||
|
|
||
| changePasswordForm.addEventListener("submit", function (event) { | ||
| event.preventDefault(); | ||
| account.changePassword(event, this, function (response) { | ||
| if (response.success) { | ||
| // clear any previous error messages | ||
| const errorLists = document.querySelectorAll("#changePasswordModal ul"); | ||
| if (errorLists.length) { | ||
| errorLists.forEach((list) => list.remove()); | ||
| } | ||
| // clear any previous success message | ||
| const previousSuccessMessage = document.querySelector("#changePasswordSuccessMessage"); | ||
| if (!previousSuccessMessage) { | ||
| // display success message | ||
| const successMessage = document.createElement("p"); | ||
| successMessage.setAttribute("id", "changePasswordSuccessMessage"); | ||
| successMessage.textContent = "Password successfully changed!"; | ||
| successMessage.setAttribute("style", "color: green;"); | ||
| successMessage.setAttribute("class", "m-0"); | ||
|
|
||
| // insert success message after submit button | ||
| const submitButton = changePasswordForm.querySelector('#changePasswordSubmit'); | ||
| submitButton.after(successMessage); | ||
| } | ||
|
|
||
| // reset form | ||
| changePasswordForm.reset(); | ||
| } else { | ||
| // display error messages | ||
| for (const elementId in response.data.data) { | ||
| const errorList = document.createElement("ul"); | ||
| const erroredInput = document.querySelector(`#${elementId}`); | ||
| erroredInput.after(errorList); | ||
| erroredInput.setAttribute("style", "border-color: red;"); | ||
| if (response.data.data[elementId].length > 0) { | ||
| for (const error in response.data.data[elementId]) { | ||
| const listItem = document.createElement("li"); | ||
| listItem.textContent = response.data.data[elementId][error]; | ||
| errorList.appendChild(listItem); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
The submit handler for the change-password form is registered inside the shown.bs.modal callback. Every time the modal is opened, a new submit listener is added, which will cause multiple AJAX submissions on later opens. Bind the handler once (e.g., on DOM ready) or remove an existing listener before adding a new one.
| $("#changePasswordModal").on("shown.bs.modal", function () { | |
| const changePasswordForm = document.querySelector("#changePasswordModal form"); | |
| changePasswordForm.addEventListener("submit", function (event) { | |
| event.preventDefault(); | |
| account.changePassword(event, this, function (response) { | |
| if (response.success) { | |
| // clear any previous error messages | |
| const errorLists = document.querySelectorAll("#changePasswordModal ul"); | |
| if (errorLists.length) { | |
| errorLists.forEach((list) => list.remove()); | |
| } | |
| // clear any previous success message | |
| const previousSuccessMessage = document.querySelector("#changePasswordSuccessMessage"); | |
| if (!previousSuccessMessage) { | |
| // display success message | |
| const successMessage = document.createElement("p"); | |
| successMessage.setAttribute("id", "changePasswordSuccessMessage"); | |
| successMessage.textContent = "Password successfully changed!"; | |
| successMessage.setAttribute("style", "color: green;"); | |
| successMessage.setAttribute("class", "m-0"); | |
| // insert success message after submit button | |
| const submitButton = changePasswordForm.querySelector('#changePasswordSubmit'); | |
| submitButton.after(successMessage); | |
| } | |
| // reset form | |
| changePasswordForm.reset(); | |
| } else { | |
| // display error messages | |
| for (const elementId in response.data.data) { | |
| const errorList = document.createElement("ul"); | |
| const erroredInput = document.querySelector(`#${elementId}`); | |
| erroredInput.after(errorList); | |
| erroredInput.setAttribute("style", "border-color: red;"); | |
| if (response.data.data[elementId].length > 0) { | |
| for (const error in response.data.data[elementId]) { | |
| const listItem = document.createElement("li"); | |
| listItem.textContent = response.data.data[elementId][error]; | |
| errorList.appendChild(listItem); | |
| } | |
| } | |
| } | |
| } | |
| }); | |
| const changePasswordForm = document.querySelector("#changePasswordModal form"); | |
| changePasswordForm.addEventListener("submit", function (event) { | |
| event.preventDefault(); | |
| account.changePassword(event, this, function (response) { | |
| if (response.success) { | |
| // clear any previous error messages | |
| const errorLists = document.querySelectorAll("#changePasswordModal ul"); | |
| if (errorLists.length) { | |
| errorLists.forEach((list) => list.remove()); | |
| } | |
| // clear any previous success message | |
| const previousSuccessMessage = document.querySelector("#changePasswordSuccessMessage"); | |
| if (!previousSuccessMessage) { | |
| // display success message | |
| const successMessage = document.createElement("p"); | |
| successMessage.setAttribute("id", "changePasswordSuccessMessage"); | |
| successMessage.textContent = "Password successfully changed!"; | |
| successMessage.setAttribute("style", "color: green;"); | |
| successMessage.setAttribute("class", "m-0"); | |
| // insert success message after submit button | |
| const submitButton = changePasswordForm.querySelector('#changePasswordSubmit'); | |
| submitButton.after(successMessage); | |
| } | |
| // reset form | |
| changePasswordForm.reset(); | |
| } else { | |
| // display error messages | |
| for (const elementId in response.data.data) { | |
| const errorList = document.createElement("ul"); | |
| const erroredInput = document.querySelector(`#${elementId}`); | |
| erroredInput.after(errorList); | |
| erroredInput.setAttribute("style", "border-color: red;"); | |
| if (response.data.data[elementId].length > 0) { | |
| for (const error in response.data.data[elementId]) { | |
| const listItem = document.createElement("li"); | |
| listItem.textContent = response.data.data[elementId][error]; | |
| errorList.appendChild(listItem); | |
| } | |
| } | |
| } | |
| } |
| const errorLists = document.querySelectorAll("#changePasswordModal ul"); | ||
| if (errorLists.length) { | ||
| errorLists.forEach((list) => list.remove()); | ||
| } |
There was a problem hiding this comment.
On successful password change you remove <ul> error lists, but you don’t clear the red border-color styling that was applied to invalid inputs. If the user fixes errors and then succeeds without closing the modal, the inputs can remain styled as invalid. Consider clearing the input error styles in the success branch (similar to the hidden.bs.modal cleanup).
| } | |
| } | |
| // reset input border color | |
| const passwordInputs = document.querySelectorAll("#changePasswordModal input[type='password']"); | |
| passwordInputs.forEach((input) => input.setAttribute("style", "")); |
| // display error messages | ||
| for (const elementId in response.data.data) { | ||
| const errorList = document.createElement("ul"); | ||
| const erroredInput = document.querySelector(`#${elementId}`); | ||
| erroredInput.after(errorList); | ||
| erroredInput.setAttribute("style", "border-color: red;"); |
There was a problem hiding this comment.
When handling a failed submission, the code appends new <ul> error lists after each input but doesn’t clear any existing error lists first. Submitting the form multiple times with errors will duplicate error messages in the modal. Clear prior error lists (and potentially reset input styles) before rendering the new errors.
pollardld
left a comment
pollardld
left a comment
There was a problem hiding this comment.
I seconded one of Copilot's comments regarding replacing a submit event handler, as opposed to adding each time a modal is opening.
Other than that, looks great!
| $("#changePasswordModal").on("shown.bs.modal", function () { | ||
| const changePasswordForm = document.querySelector("#changePasswordModal form"); | ||
|
|
||
| changePasswordForm.addEventListener("submit", function (event) { | ||
| event.preventDefault(); | ||
| account.changePassword(event, this, function (response) { | ||
| if (response.success) { | ||
| // clear any previous error messages | ||
| const errorLists = document.querySelectorAll("#changePasswordModal ul"); | ||
| if (errorLists.length) { | ||
| errorLists.forEach((list) => list.remove()); | ||
| } | ||
| // clear any previous success message | ||
| const previousSuccessMessage = document.querySelector("#changePasswordSuccessMessage"); | ||
| if (!previousSuccessMessage) { | ||
| // display success message | ||
| const successMessage = document.createElement("p"); | ||
| successMessage.setAttribute("id", "changePasswordSuccessMessage"); | ||
| successMessage.textContent = "Password successfully changed!"; | ||
| successMessage.setAttribute("style", "color: green;"); | ||
| successMessage.setAttribute("class", "m-0"); | ||
|
|
||
| // insert success message after submit button | ||
| const submitButton = changePasswordForm.querySelector('#changePasswordSubmit'); | ||
| submitButton.after(successMessage); | ||
| } | ||
|
|
||
| // reset form | ||
| changePasswordForm.reset(); | ||
| } else { | ||
| // display error messages | ||
| for (const elementId in response.data.data) { | ||
| const errorList = document.createElement("ul"); | ||
| const erroredInput = document.querySelector(`#${elementId}`); | ||
| erroredInput.after(errorList); | ||
| erroredInput.setAttribute("style", "border-color: red;"); | ||
| if (response.data.data[elementId].length > 0) { | ||
| for (const error in response.data.data[elementId]) { | ||
| const listItem = document.createElement("li"); | ||
| listItem.textContent = response.data.data[elementId][error]; | ||
| errorList.appendChild(listItem); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } | ||
| }); |
asana task
Description
Adds a form for a user to change their own password. The form requires the user to enter their old password, and the new password twice. This is the format that
PasswordChangeViewexpects. The form displays a success message, and errors. I added a "close" button to the login modal to make this option more clear, rather than just clicking out of the modal.I also deleted some code in the
loginmodule that is not used anymore.Screenshots
The login modal with the "close" button
Change Password button in the nav
Default form state
Successful form submission
Form with errors
