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.

Sign up for GitHub

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

Added support for boundary filters in kibana dashboard #1729

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Added support for boundary filters in kibana dashboard #1729

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions ...micro-ui-internals/packages/modules/utilities/src/pages/employee/IFrameInterface/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,33 @@ const IFrameInterface = (props) => {
enabled: true,
});

function addBoundaryFilters(url, filters) {
const { province, district } = filters || {};

// Determine filter type based on district and province
const filter = district
? `(query:(match_phrase:(Data.boundaryHierarchy.district.keyword:'${district}')))`
: province
? `(query:(match_phrase:(Data.boundaryHierarchy.province.keyword:'${province}')))`
: null;

// If there's a filter to apply
if (filter) {
// Match existing filters in the URL
const existingFilters = /filters:\!\((.*?)\)/.exec(url);

// Replace existing filters or append the new filter
const updatedUrl = existingFilters
? url.replace(existingFilters[0], `filters:!(${filter})`).replace(/ /g, '%20')
: url;

setUrl(updatedUrl);
} else {
// No filter to add, keep original URL
setUrl(url);
}
}
Comment on lines +29 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider architectural improvements for boundary filter feature

To enhance maintainability and reliability:

  1. Add JSDoc documentation for the boundary filter feature
  2. Implement error tracking for filter-related issues
  3. Add unit tests for the new functionality

Would you like me to:

  1. Generate JSDoc documentation for the boundary filter feature?
  2. Create a test suite for the new functionality?
  3. Implement error tracking integration?

Also applies to: 203-203

⚠️ Potential issue

Add input validation and security measures to addBoundaryFilters

The function needs several security and robustness improvements:

  1. Add input validation for the URL parameter
  2. Sanitize district/province values before using in query
  3. Use non-greedy regex matching
  4. Add error handling for malformed URLs
  5. Use proper escaping for query values

Consider applying these improvements:

 function addBoundaryFilters(url, filters) {
+  if (!url || typeof url !== 'string') {
+    console.error('Invalid URL provided to addBoundaryFilters');
+    return url;
+  }
+
   const { province, district } = filters || {};
+
+  // Sanitize input values
+  const sanitizeValue = (value) => {
+    if (!value) return null;
+    return encodeURIComponent(value.replace(/['"\\]/g, ''));
+  };
+
+  const sanitizedDistrict = sanitizeValue(district);
+  const sanitizedProvince = sanitizeValue(province);
 
   // Determine filter type based on district and province
-  const filter = district
-    ? `(query:(match_phrase:(Data.boundaryHierarchy.district.keyword:'${district}')))`
-    : province
-    ? `(query:(match_phrase:(Data.boundaryHierarchy.province.keyword:'${province}')))`
+  const filter = sanitizedDistrict
+    ? `(query:(match_phrase:(Data.boundaryHierarchy.district.keyword:'${sanitizedDistrict}')))`
+    : sanitizedProvince
+    ? `(query:(match_phrase:(Data.boundaryHierarchy.province.keyword:'${sanitizedProvince}')))`
     : null;

   if (filter) {
     try {
-      const existingFilters = /filters:\!\((.*?)\)/.exec(url);
+      const existingFilters = /filters:\!\((.*?)\)/?.exec(url);
 
       const updatedUrl = existingFilters
         ? url.replace(existingFilters[0], `filters:!(${filter})`).replace(/ /g, '%20')
         : url;
 
       setUrl(updatedUrl);
+    } catch (error) {
+      console.error('Error processing URL in addBoundaryFilters:', error);
+      setUrl(url);
+    }
   } else {
     setUrl(url);
   }
 }
📝 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
function addBoundaryFilters(url, filters) {
const { province, district } = filters || {};
// Determine filter type based on district and province
const filter = district
? `(query:(match_phrase:(Data.boundaryHierarchy.district.keyword:'${district}')))`
: province
? `(query:(match_phrase:(Data.boundaryHierarchy.province.keyword:'${province}')))`
: null;
// If there's a filter to apply
if (filter) {
// Match existing filters in the URL
const existingFilters = /filters:\!\((.*?)\)/.exec(url);
// Replace existing filters or append the new filter
const updatedUrl = existingFilters
? url.replace(existingFilters[0], `filters:!(${filter})`).replace(/ /g, '%20')
: url;
setUrl(updatedUrl);
} else {
// No filter to add, keep original URL
setUrl(url);
}
}
function addBoundaryFilters(url, filters) {
if (!url || typeof url !== 'string') {
console.error('Invalid URL provided to addBoundaryFilters');
return url;
}
const { province, district } = filters || {};
// Sanitize input values
const sanitizeValue = (value) => {
if (!value) return null;
return encodeURIComponent(value.replace(/['"\\]/g, ''));
};
const sanitizedDistrict = sanitizeValue(district);
const sanitizedProvince = sanitizeValue(province);
// Determine filter type based on district and province
const filter = sanitizedDistrict
? `(query:(match_phrase:(Data.boundaryHierarchy.district.keyword:'${sanitizedDistrict}')))`
: sanitizedProvince
? `(query:(match_phrase:(Data.boundaryHierarchy.province.keyword:'${sanitizedProvince}')))`
: null;
if (filter) {
try {
const existingFilters = /filters:\!\((.*?)\)/?.exec(url);
const updatedUrl = existingFilters
? url.replace(existingFilters[0], `filters:!(${filter})`).replace(/ /g, '%20')
: url;
setUrl(updatedUrl);
} catch (error) {
console.error('Error processing URL in addBoundaryFilters:', error);
setUrl(url);
}
} else {
setUrl(url);
}
}


const injectCustomHttpInterceptors = (iframeWindow) => {
// console.log("iframeInInterceptor",iframeWindow)
const injectCustomHttpInterceptor = () => {
Expand Down Expand Up @@ -173,6 +200,7 @@ const IFrameInterface = (props) => {
: "";
const title = pageObject?.["title"] || "";
let url = `${domain}${contextPath}`;
addBoundaryFilters(url, filters?.filters);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and state management for boundary filter integration

The current integration has potential issues:

  1. Multiple setUrl calls could lead to race conditions
  2. Missing error boundary
  3. Unclear effect dependencies

Consider refactoring the URL construction logic:

- addBoundaryFilters(url, filters?.filters);
+ try {
+   // Combine URL modifications to avoid multiple state updates
+   let processedUrl = url;
+   if (filters?.filters) {
+     processedUrl = addBoundaryFilters(processedUrl, filters.filters);
+   }
+   if (pageObject?.authToken?.enable) {
+     processedUrl = addAuthToken(processedUrl, pageObject.authToken);
+   }
+   setUrl(processedUrl);
+ } catch (error) {
+   console.error('Error processing URL with filters:', error);
+   setUrl(url);
+ }

Also, consider extracting the URL processing logic into a separate useCallback hook to better manage dependencies and prevent unnecessary re-renders.

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

if (pageObject?.authToken && pageObject?.authToken?.enable) {
const authKey = pageObject?.authToken?.key || "auth-token";
if (pageObject?.authToken?.customFun && Digit.Utils.createFunction(pageObject?.authToken?.customFun)) {
Expand Down
Loading