-
-
Notifications
You must be signed in to change notification settings - Fork 72
[feature:ui] Show upgrade progress for mass upgrade operations in real time #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: gsoc25
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress @youhaveme9!! 👏 - I found few issues while testing:
Steps to replicate:
-
Create a
buildthat can be used to trigger mass upgrade (i.e: same image will be used for two devices) -
Let's make sure one device is working properly and the second device is in
shutdown/offstate -
Initiate mass upgrade -> one will succeed, the other will keep retrying since we kept it in off state (you can configure settings to keep retries at small duration and less count)
Then you'll see class=upgrade-progress-text will contain multiple progress percentages %, which seems like an issue with how you append HTML elements there.
Please check - attached video reference below.
video-2025-09-09_17.11.44.mp4
- Also, can we modify this Django message statement that comes right after? "You need to keep refreshing the page to check progress", but it seems like we can remove that considering this page will support real-time updates using WebSockets:
text = _(
"You can track the progress of this mass upgrade operation "
"in this page. Refresh the page from time to time to check "
"its progress."
)
For selenium tests: when adding Selenium tests for this feature, make sure you also add tests for all different cases like:
- all batch upgrade processes fail
- assert percentage accuracy
- expected UI elements like progress bar colors
- mix of other possibilities such as: all
aborted,cancelled,failed,success,in-progress, and various subsets of these states
| if ( | ||
| statusText && | ||
| (statusText.includes("progress") || | ||
| statusText === "success" || | ||
| statusText === "completed successfully" || | ||
| statusText === "failed" || | ||
| statusText === "aborted") | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double-check throughout this feature to ensure we're handling the cancelled status properly in these conditionals ^^
| if ( | ||
| operation.status === "success" || | ||
| operation.status === "failed" || | ||
| operation.status === "aborted" | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double-check throughout this feature to ensure we're handling the cancelled status properly in these conditionals ^^
|
|
||
| if (status === "in-progress" || status === "in progress") { | ||
| statusHtml += ` | ||
| <div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill in-progress" style="width: ${progressPercentage}%"></div> | ||
| </div> | ||
| <span class="upgrade-progress-text">${progressPercentage}%</span> | ||
| `; | ||
| } else if (status === "success") { | ||
| statusHtml += ` | ||
| <div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill success" style="width: 100%"></div> | ||
| </div> | ||
| <span class="upgrade-progress-text">100%</span> | ||
| `; | ||
| } else if (status === "failed" || status === "aborted") { | ||
| statusHtml += ` | ||
| <div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill ${status}" style="width: 100%"></div> | ||
| </div> | ||
| `; | ||
| } else { | ||
| statusHtml += ` | ||
| <div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill" style="width: ${progressPercentage}%"></div> | ||
| </div> | ||
| <span class="upgrade-progress-text">${progressPercentage}%</span> | ||
| `; | ||
| } | ||
|
|
||
| statusContainer.html(statusHtml); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double-check throughout this feature to ensure we're handling the cancelled status properly in these conditionals ^^
| def update_batch_status(self, batch_instance): | ||
| """Update and publish batch status based on current operations""" | ||
| batch_instance.refresh_from_db() | ||
|
|
||
| if hasattr(batch_instance, "_upgrade_operations"): | ||
| delattr(batch_instance, "_upgrade_operations") | ||
|
|
||
| operations = batch_instance.upgradeoperation_set | ||
| total_operations = operations.count() | ||
| in_progress_operations = operations.filter(status="in-progress").count() | ||
| completed_operations = operations.exclude(status="in-progress").count() | ||
| failed_operations = operations.filter(status="failed").count() | ||
| successful_operations = operations.filter(status="success").count() | ||
|
|
||
| # Determine overall batch status | ||
| if in_progress_operations > 0: | ||
| batch_status = "in-progress" | ||
| elif failed_operations > 0: | ||
| batch_status = "failed" | ||
| elif ( | ||
| successful_operations > 0 | ||
| and completed_operations == total_operations | ||
| and total_operations > 0 | ||
| ): | ||
| batch_status = "success" | ||
| else: | ||
| batch_status = batch_instance.status | ||
|
|
||
| # Update the batch instance status if needed | ||
| if batch_instance.status != batch_status: | ||
| batch_instance.status = batch_status | ||
| batch_instance.save(update_fields=["status"]) | ||
|
|
||
| self.publish_batch_status(batch_status, completed_operations, total_operations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to handle cancelled and aborted cases as well?
…e-upgrader into issue/show-mass-upgrade-progress
09f9102 to
78afd62
Compare
Aryamanz29
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @youhaveme9 - I'm checking recent updates. Can you look at this behavior below:
Steps:
- Execute mass upgrade with a single device and then cancel that from the UI
- Progress bar turns
grey, which is expected, but in mass upgrade when we look at that, it'sgreen❌ and the overall status says "completed successfully," though this seems incorrect since it wascancelledby the user 🤔
Correct me if I'm wrong - are we considering user cancellation/abort in mass upgrade as a whole successful status 🟢 (similar to what shown in the video below)? For this use case where mass upgrade runs on one device, I was hoping to get the same final status as it was running as a single operation ie: grey. In case of more than one device (that's when mass upgrade is actually useful), it should give final results based on evaluating all individual upgrade operations' statuses, e.g one successful 🟢 , one failure 🔴 results in final status -> completed with some failures 🟠
@nemesifier @youhaveme9 - also, can we reuse the upgrade operation cancellation feature in the mass upgrade device view as well to be consistent? What do you think?
^^ It would be consistent UX for users to individually cancel any operation if they want to. Otherwise, I need to go to devices -> particular device -> recent firmware upgrades -> perform cancellation.
video-2025-09-16_16.45.16.mp4
@youhaveme9 is working on that in a separate PR. |
@nemesifier This feature I have already implemented in this PR |
Aryamanz29
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress - @youhaveme9 - few minor suggestions below:
| try: | ||
| auth_result = self._is_user_authenticated() | ||
| if not auth_result: | ||
| return | ||
|
|
||
| if not self.is_user_authorized(): | ||
| await self.close() | ||
| return | ||
|
|
||
| # Join room group | ||
| await self.channel_layer.group_add(self.group_name, self.channel_name) | ||
| await self.accept() | ||
| self.operation_id = self.scope["url_route"]["kwargs"]["operation_id"] | ||
| self.group_name = f"upgrade_{self.operation_id}" | ||
|
|
||
| except (AssertionError, KeyError) as e: | ||
| logger.error(f"Error in operation websocket connect: {e}") | ||
| await self.close() | ||
| else: | ||
| try: | ||
| await self.channel_layer.group_add(self.group_name, self.channel_name) | ||
| except (ConnectionError, TimeoutError) as e: | ||
| logger.error(f"Failed to add channel to group {self.group_name}: {e}") | ||
| await self.close() | ||
| return | ||
| except RuntimeError as e: | ||
| logger.error( | ||
| f"Channel layer error when joining group {self.group_name}: {e}" | ||
| ) | ||
| await self.close() | ||
| return | ||
| await self.accept() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for these websocket error-handling scenarios (with proper log assertions)? 🙏
| try: | ||
| await self.channel_layer.group_discard(self.group_name, self.channel_name) | ||
| except AttributeError: | ||
| return | ||
|
|
||
| async def receive_json(self, content): | ||
| """Handle incoming messages from the client""" | ||
| message_type = content.get("type") | ||
|
|
||
| if message_type == "request_current_state": | ||
| await self._handle_current_operation_state_request(content) | ||
| else: | ||
| logger.warning(f"Unknown message type received: {message_type}") | ||
|
|
||
| async def _handle_current_operation_state_request(self, content): | ||
| """Handle request for current state of the operation""" | ||
| UpgradeOperation = load_model("firmware_upgrader", "UpgradeOperation") | ||
| try: | ||
| # Get the operation using sync_to_async | ||
| get_operation = sync_to_async( | ||
| lambda: UpgradeOperation.objects.filter(pk=self.operation_id).first() | ||
| ) | ||
| operation = await get_operation() | ||
| if operation: | ||
| # Send operation update | ||
| await self.send_json( | ||
| { | ||
| "type": "operation_update", | ||
| "operation": { | ||
| "id": str(operation.pk), | ||
| "status": operation.status, | ||
| "log": operation.log or "", | ||
| "progress": getattr(operation, "progress", 0), | ||
| "modified": ( | ||
| operation.modified.isoformat() | ||
| if operation.modified | ||
| else None | ||
| ), | ||
| }, | ||
| } | ||
| ) | ||
| except (ConnectionError, TimeoutError) as e: | ||
| logger.error( | ||
| f"Failed to connect to channel layer during operation state request: {e}" | ||
| ) | ||
| except RuntimeError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for these websocket error-handling scenarios (with proper log assertions)? 🙏
| try: | ||
| auth_result = self._is_user_authenticated() | ||
| if not auth_result: | ||
| return | ||
| if not self.is_user_authorized(): | ||
| await self.close() | ||
| return | ||
| self.batch_id = self.scope["url_route"]["kwargs"]["batch_id"] | ||
| self.group_name = f"batch_upgrade_{self.batch_id}" | ||
|
|
||
| # Join room group | ||
| await self.channel_layer.group_add(self.group_name, self.channel_name) | ||
| await self.accept() | ||
| except (AssertionError, KeyError) as e: | ||
| logger.error(f"Error in batch websocket connect: {e}") | ||
| await self.close() | ||
| else: | ||
| try: | ||
| await self.channel_layer.group_add(self.group_name, self.channel_name) | ||
| except (ConnectionError, TimeoutError) as e: | ||
| logger.error(f"Failed to add channel to group {self.group_name}: {e}") | ||
| await self.close() | ||
| return | ||
| except RuntimeError as e: | ||
| logger.error( | ||
| f"Channel layer error when joining group {self.group_name}: {e}" | ||
| ) | ||
| await self.close() | ||
| return | ||
|
|
||
| await self.accept() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for these websocket error-handling scenarios (with proper log assertions)? 🙏
| batch_operation.upgrade_operations.all() | ||
| ) | ||
| for operation in operations_list: | ||
| await self.send_json( | ||
| { | ||
| "type": "operation_progress", | ||
| "operation_id": str(operation.pk), | ||
| "status": operation.status, | ||
| "progress": getattr(operation, "progress", 0), | ||
| "modified": ( | ||
| operation.modified.isoformat() | ||
| if operation.modified | ||
| else None | ||
| ), | ||
| } | ||
| ) | ||
| except (ConnectionError, TimeoutError) as e: | ||
| logger.error( | ||
| f"Failed to connect to channel layer during batch state request: {e}" | ||
| ) | ||
| except RuntimeError as e: | ||
| logger.error(f"Runtime error during batch state request: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for these websocket error-handling scenarios (with proper log assertions)? 🙏
| (statusText.includes("progress") || | ||
| statusText === "success" || | ||
| statusText === "completed successfully" || | ||
| statusText === "completed with some failures" || | ||
| statusText === "completed with some cancellations" || | ||
| statusText === "failed" || | ||
| statusText === "aborted" || | ||
| statusText === "cancelled") | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this pattern of string comparisons frequently in our .js files. Can we make this cleaner by using a const object (since JS doesn't have native enums)? This pattern is problematic because it's:
- error-prone (typos)
- hard to maintain
- not reusable
Proposed refactor (example):
const FW_UPGRADE_STATUS = {
SUCCESS: "success",
COMPLETED_SUCCESSFULLY: "completed successfully",
COMPLETED_WITH_FAILURES: "completed with some failures",
COMPLETED_WITH_CANCELLATIONS: "completed with some cancellations",
FAILED: "failed",
ABORTED: "aborted",
CANCELLED: "cancelled"
};
// Option 1: For individual comparisons
if (statusText === FW_UPGRADE_STATUS.SUCCESS ||
statusText === FW_UPGRADE_STATUS.COMPLETED_SUCCESSFULLY) {
// handle specific statuses
}
// Option 2: For checking any valid status
const VALID_FW_STATUSES = new Set(Object.values(FW_UPGRADE_STATUS)); // O(1) lookup with Set for validation
if (VALID_FW_STATUSES.has(statusText)) {
// handle any valid status
}| if (data.status === "success") { | ||
| progressPercentage = 100; | ||
| statusClass = "completed-successfully"; | ||
| showPercentageText = true; | ||
| } else if (data.status === "cancelled") { | ||
| progressPercentage = 100; | ||
| statusClass = "cancelled"; | ||
| showPercentageText = false; | ||
| } else if (data.status === "failed") { | ||
| let successfulOpsCount = $("#result_list tbody tr").filter(function () { | ||
| let statusText = $(this).find(".status-cell .status-content").text().trim(); | ||
| return statusText === "success" || statusText === "completed successfully"; | ||
| }).length; | ||
|
|
||
| // Also check individual operation containers for success | ||
| if (successfulOpsCount === 0) { | ||
| $("#result_list tbody tr").each(function () { | ||
| let statusContainer = $(this).find(".upgrade-status-container"); | ||
| if ( | ||
| statusContainer.length && | ||
| statusContainer.find(".upgrade-progress-fill.success").length | ||
| ) { | ||
| successfulOpsCount++; | ||
| } | ||
| }); | ||
| } | ||
| if (successfulOpsCount > 0) { | ||
| // Some operations succeeded - partial success (orange) | ||
| progressPercentage = 100; | ||
| statusClass = "partial-success"; | ||
| showPercentageText = false; | ||
| } else { | ||
| // All operations failed - total failure (red) | ||
| progressPercentage = 100; | ||
| statusClass = "failed"; | ||
| showPercentageText = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here) I see this pattern of string comparisons frequently in our .js files. Can we make this cleaner by using a const object (since JS doesn't have native enums)?
| let statusField = $(".field-status .readonly"); | ||
| if (statusField.length > 0 && data.status) { | ||
| let displayStatus = data.status; | ||
| if (data.status === "success") { | ||
| displayStatus = "completed successfully"; | ||
| } else if (data.status === "cancelled") { | ||
| displayStatus = "completed with some cancellations"; | ||
| } else if (data.status === "failed") { | ||
| displayStatus = "completed with some failures"; | ||
| } else if (data.status === "in-progress") { | ||
| displayStatus = "in progress"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here) I see this pattern of string comparisons frequently in our .js files. Can we make this cleaner by using a const object (since JS doesn't have native enums)?
| if (status === "in-progress" || status === "in progress") { | ||
| statusHtml = `<div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill in-progress" style="width: ${progressPercentage}%"></div> | ||
| </div>`; | ||
| } else if (status === "success" || status === "completed successfully") { | ||
| statusHtml = `<div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill success" style="width: 100%"></div> | ||
| </div>`; | ||
| } else if (status === "failed") { | ||
| statusHtml = `<div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill failed" style="width: 100%"></div> | ||
| </div>`; | ||
| } else if (status === "aborted") { | ||
| statusHtml = `<div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill aborted" style="width: 100%"></div> | ||
| </div>`; | ||
| } else if (status === "cancelled") { | ||
| statusHtml = `<div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill cancelled" style="width: 100%"></div> | ||
| </div>`; | ||
| } else { | ||
| statusHtml = `<div class="upgrade-progress-bar"> | ||
| <div class="upgrade-progress-fill" style="width: ${progressPercentage}%"></div> | ||
| </div>`; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here) I see this pattern of string comparisons frequently in our .js files. Can we make this cleaner by using a const object (since JS doesn't have native enums)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely many if/elses are not very elegant :)
| } | ||
| if ( | ||
| status === "completed successfully" || | ||
| status === "success" || | ||
| status === "failed" || | ||
| status === "aborted" || | ||
| status === "cancelled" | ||
| ) { | ||
| return 100; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here) I see this pattern of string comparisons frequently in our .js files. Can we make this cleaner by using a const object (since JS doesn't have native enums)?
Aryamanz29
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @youhaveme9 - Found another issue on the batch upgrade operation list page. When an upgrade operation has cancellations, it shows -> '-' in the STATUS field. However, when we open the detail page, it shows completed with some cancellations. We should reflect the same status on the list page. It looks like we need to add new status choices to the model, since the current ones don't include this status: 🤔
openwisp-firmware-upgrader/openwisp_firmware_upgrader/base/models.py
Lines 494 to 499 in cff990c
| STATUS_CHOICES = ( | |
| ("idle", _("idle")), | |
| ("in-progress", _("in progress")), | |
| ("success", _("completed successfully")), | |
| ("failed", _("completed with some failures")), | |
| ) |
e4aa43a to
413566c
Compare
pandafy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youhaveme9 we need to rebase the gsoc 25 on the current master, and then rebase this on top of it.
| const ALL_VALID_FW_STATUSES = new Set([ | ||
| ...Object.values(FW_UPGRADE_STATUS), | ||
| ...Object.values(FW_UPGRADE_DISPLAY_STATUS), | ||
| ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youhaveme9 can you please explain why do we need to combine both status values and status display labels?
| const FW_UPGRADE_DISPLAY_STATUS = { | ||
| COMPLETED_SUCCESSFULLY: "completed successfully", | ||
| COMPLETED_WITH_FAILURES: "completed with some failures", | ||
| COMPLETED_WITH_CANCELLATIONS: "completed with some cancellations", | ||
| IN_PROGRESS: "in progress", | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to mark this for translation, right?
| IN_PROGRESS: "in-progress", | ||
| IN_PROGRESS_ALT: "in progress", // Alternative representation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need alternative representation?
| if (typeof window !== "undefined") { | ||
| window.FW_UPGRADE_STATUS = FW_UPGRADE_STATUS; | ||
| window.FW_UPGRADE_DISPLAY_STATUS = FW_UPGRADE_DISPLAY_STATUS; | ||
| window.FW_UPGRADE_CSS_CLASSES = FW_UPGRADE_CSS_CLASSES; | ||
| window.VALID_FW_STATUSES = VALID_FW_STATUSES; | ||
| window.VALID_FW_DISPLAY_STATUSES = VALID_FW_DISPLAY_STATUSES; | ||
| window.ALL_VALID_FW_STATUSES = ALL_VALID_FW_STATUSES; | ||
| window.FW_STATUS_GROUPS = FW_STATUS_GROUPS; | ||
| window.FW_STATUS_HELPERS = FW_STATUS_HELPERS; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these used?
| def is_user_authorized(self): | ||
| user = self.scope["user"] | ||
| is_authorized = user.is_superuser or user.is_staff | ||
| return is_authorized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be more stringent here.
User will be deemed authorized if
- user is superuser, or
- user has permission to manage the organization of the object and user has view (or change?) permission on the model.
| # Determine overall batch status based on individual operation statuses | ||
| if in_progress_operations > 0: | ||
| batch_status = "in-progress" | ||
| elif cancelled_operations > 0: | ||
| batch_status = "cancelled" | ||
| elif failed_operations > 0 or aborted_operations > 0: | ||
| batch_status = "failed" | ||
| elif ( | ||
| successful_operations > 0 | ||
| and completed_operations == total_operations | ||
| and total_operations > 0 | ||
| ): | ||
| batch_status = "success" | ||
| else: | ||
| batch_status = batch_instance.status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Shouldn't this be something which is set at the model level?
If the user requests the objects using DRF, they will see a different result from the Django admin. Please correct me if my assumption is wrong.
| if batch_instance.status != batch_status: | ||
| batch_instance.status = batch_status | ||
| batch_instance.save(update_fields=["status"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see we are updating the status in the database, but I still think that this logic should be moved to the model definition.
| <script type="text/javascript" src="{% static 'connection/js/lib/reconnecting-websocket.min.js' %}"></script> | ||
| <script> | ||
| // Configuration for WebSocket connection | ||
| var owControllerApiHost = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| var owControllerApiHost = { | |
| var owFirmwareUpgraderApiHost = { |
| <script type="text/javascript" src="{% static 'firmware-upgrader/js/firmware-upgrade-constants.js' %}"></script> | ||
| <script type="text/javascript" src="{% static 'firmware-upgrader/js/batch-upgrade-progress.js' %}"></script> | ||
| <script> | ||
| django.jQuery(function($) { | ||
| var statusField = $('.field-status .readonly'); | ||
| if (statusField.length && !statusField.find('.batch-main-progress').length) { | ||
| statusField.append('<div class="batch-main-progress"></div>'); | ||
| } | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These scripts should be present in the footer instead of extreahead block, unless we have a verify specific reason which needs to be explained in an inline comment.
| function detectPageType() { | ||
| // Check if it's a single upgrade operation page | ||
| if (window.location.pathname.includes("/upgradeoperation/")) { | ||
| return "operation"; | ||
| } | ||
| // Check if it's a device page (with upgrade operations) | ||
| if (document.getElementById("upgradeoperation_set-group")) { | ||
| return "device"; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On first look, it looks a fragile piece of code. Instead relying on URLs or presence of some element, we should just set a JS variable in the HTML template which serves as a flag for the type of operation.
I'd be fine if we are looking for some concrete HTML element whose value does not change if the application is extended. But, window.location.pathname.includes("/upgradeoperation/") is super fragile.


Checklist
#224
Screenshot