Skip to content
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

0187-WebUI-Fix-GroupEditingIfDevicesInInbox #2886

Conversation

jp112sdl
Copy link
Contributor

@jp112sdl jp112sdl commented Oct 31, 2024

Description

Mit diesem Patch werden 3 Probleme gefixt:

  1. Wird eine Gruppe (VirtualDevice) bearbeitet, so lange sich ein "gruppenfähiges" Gerät (z.B. Thermostat, Fensterkontakt) im Posteingang befindet (und somit noch kein SetReadyConfig erfolgt ist), schlägt das Speichern mit einem Javascript Fehler fehl.
    Ursache ist, dass Geräte im Posteingang noch keine ID im WebUI Device Array besitzen.
    https://github.com/jp112sdl/RaspberryMatic/blob/f3fd04362b484f388e31746672cbdec226b716a4/buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch#L17-L20

  2. Beim Bearbeiten einer Gruppe werden im unteren Abschnitt alle Geräte aufgelistet, die zu der Gruppe hinzugefügt werden können.
    Bei Geräten, die sich noch im Posteingang befinden, fehlte das Symbolbild.
    https://github.com/jp112sdl/RaspberryMatic/blob/f3fd04362b484f388e31746672cbdec226b716a4/buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch#L24-L33

  3. Die Funktion des Buttons "Einstellen" in der Ansicht "Gruppe bearbeiten" führte zu einem Javscript Fehler selfGroup is not defined
    https://github.com/jp112sdl/RaspberryMatic/blob/f3fd04362b484f388e31746672cbdec226b716a4/buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch#L7-L9

Issues

#1437

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Alternate Designs

Possible Drawbacks

Verification Process

Release Notes

Contributing checklist

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING and LICENSE document.
  • I fully agree to distribute my changes under Apache 2.0 license.

Summary by CodeRabbit

  • New Features

    • Introduced a new HTML template for the Group Edit Page, enhancing user interaction for editing group details and managing devices.
    • Added functionalities for dynamic data binding and real-time updates using JavaScript and Knockout.js.
    • Implemented error handling and user feedback mechanisms during save operations.
  • Bug Fixes

    • Improved handling of undefined devices to prevent errors in device configuration and metadata retrieval.
  • Documentation

    • Enhanced the documentation for the Group Edit Page to reflect new functionalities and improve user guidance.

Copy link

coderabbitai bot commented Oct 31, 2024

Walkthrough

The changes in this pull request enhance the functionality of the Group Edit Page in a web application by introducing a new HTML template and JavaScript module for managing virtual devices and groups. Key updates include the addition of classes and functions for device handling, improved error checking, and dynamic data binding using Knockout.js. The modifications ensure better management of device configurations and user interactions, allowing for a more robust and interactive experience when editing device groups.

Changes

File Path Change Summary
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMserver/opt/HMServer/pages/GroupEditPage.ftl New HTML template for the Group Edit Page with a form for editing group details, including device management and dynamic user interactions through JavaScript functions.
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js New JavaScript module introducing classes (GroupDevice, Header, VirtualGroup) and functions for managing virtual devices and groups, including error handling and AJAX requests.
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js.orig Original version of the new JavaScript module before changes were applied.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant GroupEditPage
    participant DeviceList
    participant Server

    User->>GroupEditPage: Open Group Edit Page
    GroupEditPage->>DeviceList: Load Assigned Devices
    DeviceList->>GroupEditPage: Return Assigned Devices
    GroupEditPage->>User: Display Devices
    User->>GroupEditPage: Modify Group Details
    GroupEditPage->>Server: Save Group Changes
    Server-->>GroupEditPage: Confirmation
    GroupEditPage->>User: Show Save Confirmation
Loading

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 21

🧹 Outside diff range and nitpick comments (13)
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch (2)

16-20: Good defensive programming to handle inbox devices!

The added check prevents JavaScript errors when processing devices that are still in the inbox. This fixes the first reported issue where saving would fail due to missing device IDs.

Consider fixing the indentation to match the surrounding code:

 ko.utils.arrayForEach(viewModel.assignableDevices(), function(item) {
   if(item.device != undefined) {
     homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "false"});
   }
 });

1-33: Well-structured solution addressing all reported issues!

The changes follow a good architectural pattern by:

  1. Properly initializing objects before use
  2. Adding defensive checks for undefined values
  3. Providing fallback behavior for edge cases

This makes the group editing functionality more robust while handling devices in various states (configured or in inbox).

Consider adding error logging for cases where item.device is undefined to help track potential issues in the future.

buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js.orig (1)

137-137: Address TODO comment regarding virtualDeviceName usage

The comment // Todo check if this is still in use somewhere indicates that the usage of virtualDeviceName is uncertain. Unaddressed TODOs can lead to confusion and maintenance issues.

Please verify whether virtualDeviceName is used elsewhere in the codebase. If it's not used, consider removing it to clean up the code.

buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js (3)

38-41: Ensure accurate interfaceId assignment

The condition !self.type.startsWith("HM-") might not correctly determine the interfaceId for all device types, especially if new device types are added in the future.

Consider using a more robust method to set interfaceId based on device type. For example, you could implement a mapping of device type prefixes to interface IDs or use a whitelist of known types.


150-154: Redundant animation functions around showConfiguration

The calls to ShowWaitAnim() and HideWaitAnim() immediately before and after DeviceListPage.showConfiguration() may be unnecessary if showConfiguration already handles animations internally.

Review whether these animation functions are needed here. Removing redundant calls can simplify the code.


99-101: Reuse existing utility functions for padding numbers

The padLeft function may duplicate functionality provided by existing libraries or utilities.

If a standard utility or library function is available for padding numbers, consider using it to reduce code duplication.

buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMServer/opt/HMServer/pages/GroupEditPage.ftl.orig (3)

1-2: Remove commented-out HTML tags at the beginning of the file

The <html> and <head> tags are commented out. If they are no longer needed, consider removing them to clean up the code.


612-614: Add error handling for AJAX request in _SaveGroup function

The AJAX request in the _SaveGroup function lacks error handling for network failures or server errors.

Include onFailure and onException handlers to properly manage possible errors during the request.


80-84: Ensure user-friendly messages in empty states

When no assigned devices are present, the message ${groupNoMoreDevices} is displayed.

Verify that this message is clear to the user and consider providing guidance on adding devices.

buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMServer/opt/HMServer/pages/GroupEditPage.ftl (4)

23-23: Avoid inline event handlers in favor of data-bindings

Using Knockout.js data-bindings for event handlers enhances maintainability by keeping JavaScript out of the HTML markup.

Apply this diff to refactor the event handlers:

-<input type="text" onchange="checkGroupName();" onclick="storeGroupName();" id="group_name" data-bind="value: groupName" size="50"/>
+<input type="text" id="group_name" data-bind="value: groupName, event: { change: checkGroupName, click: storeGroupName }" size="50"/>

313-321: Remove commented out code to improve readability

The block of code is no longer needed and can be safely removed to enhance code clarity.

Apply this diff to remove the commented code:

-/*
-jQuery.get( "/config/ic_deviceparameters.cgi?sid="+SessionId+"&iface=VirtualDevices&address="+self.virtualDeviceSerialNumber()+"&redirect_url=GO_BACK",
-        function( data ) {
-        WebUI.previousPage        = WebUI.currentPage;
-        WebUI.previousPageOptions = WebUI.currentPageOptions;
-        WebUI.currentPage = DeviceListPage;
-        HideWaitAnim();
-        jQuery("#content").html(data);
-});
-*/

495-497: Ensure local variables are properly scoped

Variables like channelOffset are defined in multiple functions. Consider declaring them with var or let to ensure proper scoping.

Apply this diff to define variables with proper scope:

-  channelOffset = 1;
+  var channelOffset = 1;

Also applies to: 506-510


548-549: Prefer strict equality checks

Using strict equality (===) prevents unexpected type coercion, improving code reliability.

Apply this diff to use strict comparisons:

-if(item.device != undefined) {
+if(item.device !== undefined) {

Repeat for similar comparisons throughout the code.

Also applies to: 570-571

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 94e07bc and f3fd043.

📒 Files selected for processing (5)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch (1 hunks)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMServer/opt/HMServer/pages/GroupEditPage.ftl (1 hunks)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMServer/opt/HMServer/pages/GroupEditPage.ftl.orig (1 hunks)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js (1 hunks)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js.orig (1 hunks)
🧰 Additional context used
🪛 Biome
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js

[error] 46-46: Invalid typeof comparison value: this expression is not a string literal

not a string literal

(lint/suspicious/useValidTypeof)


[error] 180-180: Use a regular expression literal instead of the RegExp constructor.

Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.

(lint/complexity/useRegexLiterals)

🔇 Additional comments (6)
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch (2)

27-31: Good solution for handling device icons!

The added fallback ensures that devices always have an icon, even when they're in the inbox. This fixes the second reported issue where inbox devices were missing their icons.

Let's verify that the image paths are consistent:

✅ Verification successful

Based on the search results, I can verify that the DEV_getImagePath function is consistently used with two parameters across the codebase - the device/type identifier and the size (50). The new code follows this same pattern by using this.type and size 50.

Image path handling is consistent with codebase patterns

The added fallback uses the same parameter structure as all other DEV_getImagePath calls in the codebase, maintaining consistency in how device icons are loaded.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all DEV_getImagePath usages to ensure consistent parameters
rg "DEV_getImagePath\(" -A 2

Length of output: 60637


7-9: Good fix for the "Set" button JavaScript error!

The change properly initializes a virtual device before accessing its ID, preventing the selfGroup is not defined error. This is a more robust approach.

Let's verify that the virtual device creation works as expected:

✅ Verification successful

The virtual device creation is consistent with existing usage

The change properly initializes a virtual device using the GroupDevice constructor with the same parameters (id, serialNumber, type) as used throughout the codebase. The constructor implementation shows it correctly handles the device initialization and sets up required properties like device.id that's used in DeviceListPage.showConfiguration().

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other instances where virtual devices are created to ensure consistency
ast-grep --pattern 'new GroupDevice($_, $_, $_)'

Length of output: 60637


Script:

#!/bin/bash
# Search for GroupDevice class and its usage
rg -A 5 "class GroupDevice" 
rg -A 5 "new GroupDevice"

Length of output: 27289


Script:

#!/bin/bash
# Search for GroupDevice class definition
rg -A 10 "function GroupDevice" 

Length of output: 3575

buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/WebUI/www/webui/js/common/viewmodels.js.orig (2)

38-38: 🛠️ Refactor suggestion

Ensure compatibility of startsWith method across browsers

The String.prototype.startsWith method may not be supported in older browsers like Internet Explorer 11. To ensure broader compatibility, consider using indexOf or adding a polyfill for startsWith.

Apply this diff to use indexOf for compatibility:

-if(!self.type.startsWith("HM-"))
+if(self.type.indexOf("HM-") !== 0)

Alternatively, if you prefer to use startsWith, include a polyfill to support older browsers.


169-169: ⚠️ Potential issue

Undefined variables SessionId and pb

The variables SessionId and pb are used but not defined within this scope. This will cause a ReferenceError at runtime.

Please ensure that SessionId and pb are properly defined or passed into this function. Verify that they are accessible in the current context.

If pb is intended to be an empty object for POST data, consider initializing it as an object:

-var pb = "{}";
+var pb = {};

Also applies to: 171-171

buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMServer/opt/HMServer/pages/GroupEditPage.ftl.orig (2)

450-456: Review callback logic in adaptChannelNames function

The function uses callbacks inside a loop, which may lead to issues with asynchronous execution and index tracking.

Ensure that DeviceList.beginUpdateDevice is called after all channels have been renamed by implementing proper completion checks.


443-457: Handle device types consistently in adaptChannelNames function

The logic for setting channelOffset based on device.typeName might not cover all device types.

Review the device types and ensure that channelOffset is correctly determined for all relevant devices.

//Class to represent a device
function GroupDevice(id, serialNumber, type)
{
conInfo(id + " " + serialNumber + " " + type);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use of undefined function conInfo

The function conInfo is used for logging but is not defined within the provided code. This will result in a runtime error if conInfo is not defined elsewhere.

If conInfo is intended to log information, consider defining it or using console.info:

-conInfo(id + " " + serialNumber + " " + type);
+console.info(id + " " + serialNumber + " " + type);

-conInfo(self.realSerialNumber);
+console.info(self.realSerialNumber);

-conInfo(self.device);
+console.info(self.device);

Ensure that all logging functions are properly defined or use standard console methods.

Also applies to: 12-12, 15-15

var chn = DeviceList.getChannelByAddress(serialNumber);
if (typeof chn != "undefined") {
self.name = chn.getName();
self.extDescr = (typeof chn.nameExtention != "undefined") ? chn.nameExtention : "";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct typo in property name nameExtention

There appears to be a typo in the property nameExtention. The correct spelling should be nameExtension.

Apply this diff to fix the typo:

-self.extDescr = (typeof chn.nameExtention != "undefined") ? chn.nameExtention : "";
+self.extDescr = (typeof chn.nameExtension != "undefined") ? chn.nameExtension : "";

Also, verify that the property nameExtension exists and is correctly spelled in all related code.

📝 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
self.extDescr = (typeof chn.nameExtention != "undefined") ? chn.nameExtention : "";
self.extDescr = (typeof chn.nameExtension != "undefined") ? chn.nameExtension : "";

@@ -0,0 +1,191 @@
virtualDevicePrefix = "INT";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Declare 'virtualDevicePrefix' using var, let, or const to prevent global scope

Declaring virtualDevicePrefix without a declaration keyword makes it a global variable, which can lead to unintended side effects and conflicts in the global namespace. It's best practice to declare variables properly to maintain scope and avoid potential issues.

Apply this diff to declare the variable properly:

-virtualDevicePrefix = "INT";
+var virtualDevicePrefix = "INT";
📝 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
virtualDevicePrefix = "INT";
var virtualDevicePrefix = "INT";

var deviceParamSet = homematic('Interface.getParamset', {"interface": interfaceId, "address" : self.realSerialNumber+":0", "paramsetKey" : "VALUES"});
// conInfo(deviceParamSet);
// handle null/undefined like no config pending
if(deviceParamSet != typeof 'undefined' && deviceParamSet != null && (deviceParamSet.CONFIG_PENDING === '1' || deviceParamSet.CONFIG_PENDING === true))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect check for undefined in deviceParamSet

The condition deviceParamSet != typeof 'undefined' compares deviceParamSet to the string "undefined", which is not correct. The proper way to check if a variable is undefined is to use typeof deviceParamSet !== 'undefined'.

Apply this diff to correct the condition:

-if(deviceParamSet != typeof 'undefined' && deviceParamSet != null && (deviceParamSet.CONFIG_PENDING === '1' || deviceParamSet.CONFIG_PENDING === true))
+if(typeof deviceParamSet !== 'undefined' && deviceParamSet != null && (deviceParamSet.CONFIG_PENDING === '1' || deviceParamSet.CONFIG_PENDING === true))
📝 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
if(deviceParamSet != typeof 'undefined' && deviceParamSet != null && (deviceParamSet.CONFIG_PENDING === '1' || deviceParamSet.CONFIG_PENDING === true))
if(typeof deviceParamSet !== 'undefined' && deviceParamSet != null && (deviceParamSet.CONFIG_PENDING === '1' || deviceParamSet.CONFIG_PENDING === true))


selfGroup.operateVirtualDevice = function()
{
var virtualDevice = new GroupDevice(selfGroup.virtualDeviceSerialNumber,selfGroup.virtualDeviceSerialNumber);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect number of arguments when instantiating GroupDevice

The constructor GroupDevice(id, serialNumber, type) expects three arguments, but only two are provided. This can lead to unexpected behavior or errors.

Apply this diff to include the missing type argument:

-var virtualDevice = new GroupDevice(selfGroup.virtualDeviceSerialNumber,selfGroup.virtualDeviceSerialNumber);
+var virtualDevice = new GroupDevice(selfGroup.virtualDeviceSerialNumber, selfGroup.virtualDeviceSerialNumber, virtualDeviceType);

Ensure that virtualDeviceType is defined in the current scope before using it.

Committable suggestion was skipped due to low confidence.

Comment on lines 323 to 342
self.operateVirtualDevice = function()
{
var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(),self.virtualDeviceSerialNumber(), "VirtualDevice");
var url = '/pages/tabs/control/devices.htm?sid='+SessionId;
var pb = "{}";
var opt =
{
postBody: pb,
onComplete: function(t)
{
WebUI.previousPage = WebUI.currentPage;
WebUI.previousPageOptions = WebUI.currentPageOptions;
WebUI.currentPage = ControlDevicesPage;
var regularExpression = new RegExp("loadChannels\\W\\s[0-9]+\\s\\W");
var newContent = t.responseText.replace(regularExpression, "loadChannels("+virtualDevice.device.id+")");
jQuery("#content").html(newContent);
}
};
new Ajax.Request(url,opt);
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure virtualDevice has a device property with an id

In the self.operateVirtualDevice function, virtualDevice.device.id is used, but it's not clear if virtualDevice has a device property.

Verify that GroupDevice instances have a device property with an id. If not, adjust the code accordingly:

-                var newContent = t.responseText.replace(regularExpression, "loadChannels("+virtualDevice.device.id+")");
+                var newContent = t.responseText.replace(regularExpression, "loadChannels("+virtualDevice.id+")");

Committable suggestion was skipped due to low confidence.

Comment on lines 404 to 413
ReturnToListWithoutSave = function()
{
var tempPreviousPage = WebUI.previousPage;
var tempPreviousPageOptions = WebUI.previousPageOptions;
WebUI.previousPage = WebUI.currentPage;
WebUI.previousPageOptions = WebUI.currentPageOptions;
WebUI.currentPage = tempPreviousPage;
WebUI.currentPageOptions = tempPreviousPageOptions;
WebUI.goBack();
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Encapsulate functions to prevent global scope pollution

Defining functions in the global scope can lead to naming conflicts and maintenance challenges. Encapsulating them improves code organization.

Apply this diff to encapsulate the functions within the GroupViewModel:

+// Encapsulate functions within GroupViewModel
 function GroupViewModel() {
     var self = this;
     // Existing code...

+    self.ReturnToListWithoutSave = function() {
+        // Function body...
+    };
+
+    self.ReturnToListAfterSave = function(devicesInPendingState) {
+        // Function body...
+    };
+
+    self.adaptChannelNames = function(id, groupName) {
+        // Function body...
+    };
+
+    self.refreshDeviceFromHomematic = function(serialNumberParam, groupName) {
+        // Function body...
+    };
+
+    self.SaveGroup = function() {
+        // Function body...
+    };
+
+    self._SaveGroup = function() {
+        // Function body...
+    };
+
+    self.SetOperateGroupOnly = function(item, mode) {
+        // Function body...
+    };
 }
 
 // Replace global function calls with references to viewModel
-ReturnToListWithoutSave = function() { /* ... */ };
-ReturnToListAfterSave = function(devicesInPendingState) { /* ... */ };
-adaptChannelNames = function(id, groupName) { /* ... */ };
-refreshDeviceFromHomematic = function(serialNumberParam, groupName) { /* ... */ };
-SaveGroup = function() { /* ... */ };
-_SaveGroup = function() { /* ... */ };
-SetOperateGroupOnly = function(item, mode) { /* ... */ };

Also applies to: 415-440, 442-493, 494-618, 620-626

Comment on lines 260 to 274
onComplete: function(t)
{
var parsed = JSON.parse(t.responseText);
viewModel.assignableDevices.removeAll();
viewModel.assignedDevices.removeAll();
viewModel.leftoverDevices.removeAll();
ko.utils.arrayForEach(parsed.assignableGroupMembers, function(item) {
viewModel.assignableDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
});
ko.utils.arrayForEach(parsed.leftoverGroupMembers, function(item) {
viewModel.leftoverDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
});
translateButtons("btnAdd");
}
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for homematic API calls

Calls to the homematic function should handle errors to ensure robustness, especially when the API may fail or return unexpected results.

Apply this diff to handle errors in homematic calls:

 homematic("Device.getReGaIDByAddress", {address: serialNumberParam}, function(result) {
     if(changeVirtualDevice) {
         // Existing code...
     }
+}, function(error) {
+    alert(translateKey('errorHomematicCallFailed') + ": " + error);
 });

Repeat similar error handling for other homematic calls.

Also applies to: 558-614


⚠️ Potential issue

Add error handling for AJAX requests

Currently, the AJAX request lacks error handling for network failures or server errors. This may lead to silent failures without user feedback.

Apply this diff to include error handling:

 var opt =
 {
     postBody: pb,
     onComplete: function(t)
     {
         var parsed = JSON.parse(t.responseText);
         viewModel.assignableDevices.removeAll();
         viewModel.assignedDevices.removeAll();
         viewModel.leftoverDevices.removeAll();
         ko.utils.arrayForEach(parsed.assignableGroupMembers, function(item) {
             viewModel.assignableDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
         });
         ko.utils.arrayForEach(parsed.leftoverGroupMembers, function(item) {
             viewModel.leftoverDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
         });
         translateButtons("btnAdd");
+    },
+    onFailure: function(t)
+    {
+        alert(translateKey('errorAjaxRequestFailed'));
+    },
+    onException: function(t, exception)
+    {
+        alert(translateKey('errorAjaxException') + ": " + exception);
+    }
 };
📝 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
onComplete: function(t)
{
var parsed = JSON.parse(t.responseText);
viewModel.assignableDevices.removeAll();
viewModel.assignedDevices.removeAll();
viewModel.leftoverDevices.removeAll();
ko.utils.arrayForEach(parsed.assignableGroupMembers, function(item) {
viewModel.assignableDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
});
ko.utils.arrayForEach(parsed.leftoverGroupMembers, function(item) {
viewModel.leftoverDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
});
translateButtons("btnAdd");
}
};
onComplete: function(t)
{
var parsed = JSON.parse(t.responseText);
viewModel.assignableDevices.removeAll();
viewModel.assignedDevices.removeAll();
viewModel.leftoverDevices.removeAll();
ko.utils.arrayForEach(parsed.assignableGroupMembers, function(item) {
viewModel.assignableDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
});
ko.utils.arrayForEach(parsed.leftoverGroupMembers, function(item) {
viewModel.leftoverDevices.push(new GroupDevice(item.id, item.serialNumber, item.type));
});
translateButtons("btnAdd");
},
onFailure: function(t)
{
alert(translateKey('errorAjaxRequestFailed'));
},
onException: function(t, exception)
{
alert(translateKey('errorAjaxException') + ": " + exception);
}
};

Comment on lines 548 to 550
if(item.device != undefined) {
homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "false"});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure item.device and its properties are defined before access

There's a risk of accessing undefined properties if item.device or item.device.id is not defined.

Apply this diff to enhance the null checking:

 if(item.device != undefined) {
+  if(item.device.id != undefined) {
     homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "false"});
+  }
 }
📝 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
if(item.device != undefined) {
homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "false"});
}
if(item.device != undefined) {
if(item.device.id != undefined) {
homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "false"});
}
}

var data = new Object();

data.groupId = viewModel.groupId;
data.groupName = escape(viewModel.groupName());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Sanitize user input to prevent XSS

When saving groupName, ensure it's properly sanitized to protect against cross-site scripting attacks.

Apply this diff to sanitize the input:

-data.groupName = escape(viewModel.groupName());
+data.groupName = escapeHTML(viewModel.groupName());

Ensure that escapeHTML is a function that properly encodes HTML entities.

Committable suggestion was skipped due to low confidence.

@jens-maus jens-maus added 🐛 bug-report Something isn't working 🏷️ WebUI This refs the WebUI component labels Nov 1, 2024
@jens-maus jens-maus added this to the next release milestone Nov 1, 2024
@jens-maus jens-maus linked an issue Nov 1, 2024 that may be closed by this pull request
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (1)
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMserver/opt/HMServer/pages/GroupEditPage.ftl.orig (1)

257-257: Add missing semicolon after isSingleOperationForbidden assignment

The statement assigning self.isSingleOperationForbidden is missing a semicolon, which can lead to unexpected issues in some JavaScript engines or during minification. It's good practice to terminate statements with semicolons for consistency and to avoid potential errors.

Apply this diff:

- self.isSingleOperationForbidden = ${isSingleOperationForbidden}
+ self.isSingleOperationForbidden = ${isSingleOperationForbidden};
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1708129 and d726437.

📒 Files selected for processing (3)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch (1 hunks)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMserver/opt/HMServer/pages/GroupEditPage.ftl (1 hunks)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMserver/opt/HMServer/pages/GroupEditPage.ftl.orig (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox.patch
🔇 Additional comments (1)
buildroot-external/patches/occu/0187-WebUI-Fix-GroupEditingIfDevicesInInbox/occu/HMserver/opt/HMServer/pages/GroupEditPage.ftl.orig (1)

27-28: Ensure user inputs are properly validated and sanitized

The input field for the group name allows user input, which needs to be carefully validated and sanitized to prevent security issues such as injection attacks. Although there is a function to replace double quotes with single quotes, consider implementing more comprehensive validation.

[security]

Consider enhancing input sanitization:

- <input type="text" onchange="checkGroupName();" onclick="storeGroupName();" id="group_name" data-bind="value: groupName" size="50"/>
+ <input type="text" onchange="validateAndStoreGroupName();" id="group_name" data-bind="value: groupName" size="50"/>

Implement a validateAndStoreGroupName() function that validates the input against a whitelist of allowed characters and enforces length restrictions.

Comment on lines +200 to +207
this.groupNameHasChanged = false;
// TWIST-589
// Replaces the char " by '
function checkGroupName() {
var groupNameElem = jQuery("#group_name");
groupNameElem.val(groupNameElem.val().replace(/"/g, "'"));
this.groupNameHasChanged = true;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the scope of groupNameHasChanged

In the functions checkGroupName() and storeGroupName(), the use of this.groupNameHasChanged may not refer to the intended viewModel context, potentially leading to unexpected behavior. To ensure the flag is correctly updated, use viewModel.groupNameHasChanged instead.

Apply this diff to fix the scope:

- this.groupNameHasChanged = false;
+ viewModel.groupNameHasChanged = false;

In checkGroupName() function:

- this.groupNameHasChanged = true;
+ viewModel.groupNameHasChanged = true;

Also applies to: 209-213

Comment on lines +489 to +513
homematic("Device.setName", {id: result, name: groupName}, function() {
homematic("Device.listAllDetail", null, function(deviceList) {
jQuery.each(deviceList, function (index, data) {
if (data !== null)
{
var serialNumber = data["address"];
if(serialNumber == serialNumberParam)
{
DeviceList.beginUpdateDevice(data["id"], function() {
//change the channelnames of the new created device
adaptChannelNames(data["id"], groupName) ;
});
}
}
});
window.setTimeout(function() {
WebUI.enter(CreateGroupPage);
homematic("system.saveObjectModel", {}, function () {
conInfo("ObjectModel saved");
});
}, 2500);

});
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle asynchronous operations with proper callbacks

In the refreshDeviceFromHomematic function, there are nested asynchronous calls that may lead to callback hell and make the code hard to maintain. Consider using chained promises or async/await patterns (if supported) to improve readability and error handling.

Refactor the code to use promises:

function refreshDeviceFromHomematic(serialNumberParam, groupName) {
  var changeVirtualDevice = viewModel.changeVirtualDeviceName;
  homematic("Device.getReGaIDByAddress", { address: serialNumberParam })
    .then(function (result) {
      if (changeVirtualDevice) {
        // Set Name of the device
        return homematic("Device.setName", { id: result, name: groupName });
      }
    })
    .then(function () {
      return homematic("Device.listAllDetail", null);
    })
    .then(function (deviceList) {
      jQuery.each(deviceList, function (index, data) {
        // ... rest of the code
      });
      WebUI.enter(CreateGroupPage);
      return homematic("system.saveObjectModel", {});
    })
    .then(function () {
      conInfo("ObjectModel saved");
    })
    .catch(function (error) {
      // Handle errors
      console.error(error);
    });
}

Ensure that the environment supports promises or include a polyfill if necessary.

{
ShowWaitAnim();

DeviceListPage.showConfiguration(false, 'DEVICE', selfGroup.device.id);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix undefined selfGroup variable in configureVirtualDevice

The variable selfGroup is not defined within the scope of the configureVirtualDevice function, which will cause a JavaScript error when this function is called. To correctly reference the view model instance, replace selfGroup with self.

Apply this diff to fix the error:

- DeviceListPage.showConfiguration(false, 'DEVICE', selfGroup.device.id);
+ DeviceListPage.showConfiguration(false, 'DEVICE', self.device.id);
📝 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
DeviceListPage.showConfiguration(false, 'DEVICE', selfGroup.device.id);
DeviceListPage.showConfiguration(false, 'DEVICE', self.device.id);

var data = new Object();

data.groupId = viewModel.groupId;
data.groupName = escape(viewModel.groupName());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid using escape(); use proper encoding for groupName

The use of the deprecated escape() function can lead to inconsistent encoding of groupName. It's recommended to use encodeURIComponent() for URL encoding or ensure proper sanitization depending on the context where groupName will be used.

Apply this diff to update the encoding method:

- data.groupName = escape(viewModel.groupName());
+ data.groupName = encodeURIComponent(viewModel.groupName());

Ensure that the server-side code correctly decodes the groupName parameter.

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +504 to +510
window.setTimeout(function() {
WebUI.enter(CreateGroupPage);
homematic("system.saveObjectModel", {}, function () {
conInfo("ObjectModel saved");
});
}, 2500);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid using fixed delays with setTimeout; use callbacks to ensure proper execution order

Using window.setTimeout with a fixed delay of 2500 milliseconds may result in unreliable behavior if the asynchronous operations take longer than expected. This can cause WebUI.enter(CreateGroupPage) and subsequent functions to execute before the previous operations have completed. It's better to chain the asynchronous calls using callbacks or promises to ensure they execute in the correct order.

Consider refactoring the code as follows:

- window.setTimeout(function() {
-   WebUI.enter(CreateGroupPage);
-   homematic("system.saveObjectModel", {}, function () {
-     conInfo("ObjectModel saved");
-   });
- }, 2500);
+ WebUI.enter(CreateGroupPage);
+ homematic("system.saveObjectModel", {}, function () {
+   conInfo("ObjectModel saved");
+   // Any additional code that depends on the save operation can go here.
+ });

Ensure that any dependent code is placed within the callback to homematic("system.saveObjectModel", ...) to maintain the correct execution sequence.

📝 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
window.setTimeout(function() {
WebUI.enter(CreateGroupPage);
homematic("system.saveObjectModel", {}, function () {
conInfo("ObjectModel saved");
});
}, 2500);
WebUI.enter(CreateGroupPage);
homematic("system.saveObjectModel", {}, function () {
conInfo("ObjectModel saved");
// Any additional code that depends on the save operation can go here.
});

Comment on lines +641 to +647
SetOperateGroupOnly = function(item, mode) {
homematic("Device.setOperateGroupOnly", {id:item.device.id, mode: mode}, function(result) {
if (DeviceList.devices[item.device.id]) {
DeviceList.devices[item.device.id].isOperateGroupOnly = mode;
}
});
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure function SetOperateGroupOnly is properly scoped

The function SetOperateGroupOnly is defined without a declaration keyword, which can lead to it being attached to the global scope unintentionally. This can cause conflicts or overwrites in the global namespace. To prevent this, declare the function using the function keyword or assign it to a variable.

Apply this diff to define the function correctly:

- SetOperateGroupOnly = function(item, mode) {
+ function SetOperateGroupOnly(item, mode) {

Alternatively, if you intend to assign it to a variable:

- SetOperateGroupOnly = function(item, mode) {
+ var SetOperateGroupOnly = function(item, mode) {
📝 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
SetOperateGroupOnly = function(item, mode) {
homematic("Device.setOperateGroupOnly", {id:item.device.id, mode: mode}, function(result) {
if (DeviceList.devices[item.device.id]) {
DeviceList.devices[item.device.id].isOperateGroupOnly = mode;
}
});
};
function SetOperateGroupOnly(item, mode) {
homematic("Device.setOperateGroupOnly", {id:item.device.id, mode: mode}, function(result) {
if (DeviceList.devices[item.device.id]) {
DeviceList.devices[item.device.id].isOperateGroupOnly = mode;
}
});
};

Comment on lines +332 to +334
var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(),self.virtualDeviceSerialNumber(), "VirtualDevice");
DeviceListPage.showConfiguration(false, 'DEVICE', virtualDevice.device.id);
HideWaitAnim();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure virtualDevice.device.id is defined before use in configureVirtualDevice

In configureVirtualDevice, virtualDevice.device.id is accessed. Confirm that virtualDevice.device and virtualDevice.device.id are properly initialized to prevent potential undefined reference errors.

Apply this diff to check for virtualDevice.device before usage:

var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(), self.virtualDeviceSerialNumber(), "VirtualDevice");
+if (virtualDevice.device && virtualDevice.device.id) {
    DeviceListPage.showConfiguration(false, 'DEVICE', virtualDevice.device.id);
+} else {
+    console.error("virtualDevice.device.id is undefined");
+}
📝 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
var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(),self.virtualDeviceSerialNumber(), "VirtualDevice");
DeviceListPage.showConfiguration(false, 'DEVICE', virtualDevice.device.id);
HideWaitAnim();
var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(),self.virtualDeviceSerialNumber(), "VirtualDevice");
if (virtualDevice.device && virtualDevice.device.id) {
DeviceListPage.showConfiguration(false, 'DEVICE', virtualDevice.device.id);
} else {
console.error("virtualDevice.device.id is undefined");
}
HideWaitAnim();

Comment on lines +566 to +569
ko.utils.arrayForEach(viewModel.assignedDevices(), function(item) {
data.assignedDevicesIds.push(item.id);
homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "true"});
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Check for undefined item.device before accessing item.device.id

In the loop over viewModel.assignedDevices(), ensure that item.device is defined before accessing item.device.id to prevent potential runtime errors when item.device is undefined.

Apply this diff to add a check:

ko.utils.arrayForEach(viewModel.assignedDevices(), function(item) {
+    if (item.device != undefined) {
        data.assignedDevicesIds.push(item.id);
        homematic("Interface.setMetadata", {"objectId": item.device.id, "dataId": "inHeatingGroup", "value": "true"});
+    }
});
📝 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
ko.utils.arrayForEach(viewModel.assignedDevices(), function(item) {
data.assignedDevicesIds.push(item.id);
homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "true"});
});
ko.utils.arrayForEach(viewModel.assignedDevices(), function(item) {
if (item.device != undefined) {
data.assignedDevicesIds.push(item.id);
homematic("Interface.setMetadata", {"objectId":item.device.id, "dataId": "inHeatingGroup", "value" : "true"});
}
});

Comment on lines +652 to +659
var s = "";
s += "<table cellspacing='8'>";
s += "<tr>";
s += "<td align='center' valign='middle'><div class='FooterButton' onclick='ReturnToListWithoutSave();'>"+ translateKey('footerBtnCancel') +"</div></td>";
s += "<td align='center' valign='middle'><div class='FooterButton' onclick='SaveGroup();'>"+ translateKey('footerBtnOk') +"</div></td>";
s += "</tr>";
s += "</table>";
setFooter(s);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Encapsulate the global variable s to prevent namespace pollution

To avoid polluting the global namespace, consider wrapping the code that initializes and uses the variable s within an immediately invoked function expression (IIFE) or relocate it inside an existing function.

Apply this refactor:

-var s = "";
-s += "<table cellspacing='8'>";
-s += "<tr>";
-s += "<td align='center' valign='middle'><div class='FooterButton' onclick='ReturnToListWithoutSave();'>"+ translateKey('footerBtnCancel') +"</div></td>";
-s += "<td align='center' valign='middle'><div class='FooterButton' onclick='SaveGroup();'>"+ translateKey('footerBtnOk') +"</div></td>";
-s += "</tr>";
-s += "</table>";
-setFooter(s);
+(function() {
+    var s = "";
+    s += "<table cellspacing='8'>";
+    s += "<tr>";
+    s += "<td align='center' valign='middle'><div class='FooterButton' onclick='ReturnToListWithoutSave();'>"+ translateKey('footerBtnCancel') +"</div></td>";
+    s += "<td align='center' valign='middle'><div class='FooterButton' onclick='SaveGroup();'>"+ translateKey('footerBtnOk') +"</div></td>";
+    s += "</tr>";
+    s += "</table>";
+    setFooter(s);
+})();
📝 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
var s = "";
s += "<table cellspacing='8'>";
s += "<tr>";
s += "<td align='center' valign='middle'><div class='FooterButton' onclick='ReturnToListWithoutSave();'>"+ translateKey('footerBtnCancel') +"</div></td>";
s += "<td align='center' valign='middle'><div class='FooterButton' onclick='SaveGroup();'>"+ translateKey('footerBtnOk') +"</div></td>";
s += "</tr>";
s += "</table>";
setFooter(s);
(function() {
var s = "";
s += "<table cellspacing='8'>";
s += "<tr>";
s += "<td align='center' valign='middle'><div class='FooterButton' onclick='ReturnToListWithoutSave();'>"+ translateKey('footerBtnCancel') +"</div></td>";
s += "<td align='center' valign='middle'><div class='FooterButton' onclick='SaveGroup();'>"+ translateKey('footerBtnOk') +"</div></td>";
s += "</tr>";
s += "</table>";
setFooter(s);
})();

Comment on lines +350 to +367
var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(),self.virtualDeviceSerialNumber(), "VirtualDevice");
var url = '/pages/tabs/control/devices.htm?sid='+SessionId;
var pb = "{}";
var opt =
{
postBody: pb,
onComplete: function(t)
{
WebUI.previousPage = WebUI.currentPage;
WebUI.previousPageOptions = WebUI.currentPageOptions;
WebUI.currentPage = ControlDevicesPage;
var regularExpression = new RegExp("loadChannels\\W\\s[0-9]+\\s\\W");
var newContent = t.responseText.replace(regularExpression, "loadChannels("+virtualDevice.device.id+")");
jQuery("#content").html(newContent);
}
};
new Ajax.Request(url,opt);
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure virtualDevice.device.id is defined before use in operateVirtualDevice

In operateVirtualDevice, virtualDevice.device.id is used in the loadChannels function. Verify that virtualDevice.device and virtualDevice.device.id are properly initialized to avoid potential undefined reference errors.

Apply this diff to add a check:

var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(), self.virtualDeviceSerialNumber(), "VirtualDevice");
+if (virtualDevice.device && virtualDevice.device.id) {
    var url = '/pages/tabs/control/devices.htm?sid='+SessionId;
    var pb = "{}";
    var opt = {
        postBody: pb,
        onComplete: function(t) {
            WebUI.previousPage        = WebUI.currentPage;
            WebUI.previousPageOptions = WebUI.currentPageOptions;
            WebUI.currentPage = ControlDevicesPage;
            var regularExpression = new RegExp("loadChannels\\W\\s[0-9]+\\s\\W");
            var newContent = t.responseText.replace(regularExpression, "loadChannels("+virtualDevice.device.id+")");
            jQuery("#content").html(newContent);
        }
    };
    new Ajax.Request(url,opt);
+} else {
+    console.error("virtualDevice.device.id is undefined");
+}
📝 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
var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(),self.virtualDeviceSerialNumber(), "VirtualDevice");
var url = '/pages/tabs/control/devices.htm?sid='+SessionId;
var pb = "{}";
var opt =
{
postBody: pb,
onComplete: function(t)
{
WebUI.previousPage = WebUI.currentPage;
WebUI.previousPageOptions = WebUI.currentPageOptions;
WebUI.currentPage = ControlDevicesPage;
var regularExpression = new RegExp("loadChannels\\W\\s[0-9]+\\s\\W");
var newContent = t.responseText.replace(regularExpression, "loadChannels("+virtualDevice.device.id+")");
jQuery("#content").html(newContent);
}
};
new Ajax.Request(url,opt);
};
var virtualDevice = new GroupDevice(self.virtualDeviceSerialNumber(), self.virtualDeviceSerialNumber(), "VirtualDevice");
if (virtualDevice.device && virtualDevice.device.id) {
var url = '/pages/tabs/control/devices.htm?sid='+SessionId;
var pb = "{}";
var opt = {
postBody: pb,
onComplete: function(t) {
WebUI.previousPage = WebUI.currentPage;
WebUI.previousPageOptions = WebUI.currentPageOptions;
WebUI.currentPage = ControlDevicesPage;
var regularExpression = new RegExp("loadChannels\\W\\s[0-9]+\\s\\W");
var newContent = t.responseText.replace(regularExpression, "loadChannels("+virtualDevice.device.id+")");
jQuery("#content").html(newContent);
}
};
new Ajax.Request(url,opt);
} else {
console.error("virtualDevice.device.id is undefined");
}

@jens-maus jens-maus merged commit 66b8cbb into jens-maus:master Nov 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug-report Something isn't working 🏷️ WebUI This refs the WebUI component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

groups can't be changed if there is a new device in the inbox
2 participants