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

Feat: Update searchText from stringbase to regexbase #5482

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

homelab-alpha
Copy link
Contributor

@homelab-alpha homelab-alpha commented Dec 29, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Feature Request

This PR addresses the following feature request:

Requested improvement: Allow search to include hostname and URL fields, in addition to the friendly name.

Fixes

This PR also fixes the following issue:

Problem

When modifying a monitor type and saving changes, inconsistent data is stored in the database. For example:

  1. Create a new monitor:
    • Monitor Type: https
    • Friendly Name: Test ONE
    • URL: https://google.fr
    • Do not save the configuration.
  2. Change monitor type:
    • Monitor Type: Ping
    • Friendly Name: Test ONE
    • Hostname: cloudflare.com
  3. Save configuration.

Result:

  • URL https://google.fr is still stored for monitor type https.
  • Hostname cloudflare.com is stored for monitor type Ping.

Searching for .fr or fr using regexbase incorrectly includes Test ONE due to retained database values.

Solution

A default value is now provided for monitor.url when switching monitor types. If this.monitor?.hostname is present, it defaults to the value used by Uptime Kuma (https://).

Request for Feedback

Do you agree with this solution? If yes, could you test it to confirm?

Type of change

  • New feature (non-breaking change that adds functionality)
  • Bug fix (non-breaking change that resolves an issue)
  • User interface (UI)

Checklist

  • My code adheres to the project's style guidelines.
  • I ran ESLint and other linters on modified files.
  • I have thoroughly reviewed and tested my code.
  • I have added comments, particularly for complex logic (including JSDoc for methods).
  • My changes introduce no new warnings.
  • I added automated tests for my code (if applicable).

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

homelab-alpha and others added 2 commits December 29, 2024 07:30
- Enhanced the search logic to support regular expressions with case-insensitive matching.
- Escaped special characters in the search text to prevent invalid regex patterns.
- Expanded search coverage to include `monitor.url`, `monitor.hostname`, and `monitor.dns_resolve_server`.
- Added error handling to catch and log invalid regex patterns, ensuring robustness.
- Maintained compatibility with existing search fields, such as monitor name and tags.

modified: src/components/MonitorList.vue

Co-authored-by: Ionys <[email protected]>
- Added optional chaining to safely access `this.monitor.hostname`.
- Introduced default URL (`https://`) assignment to `this.monitor.url`.
- Ensured `hostname` is trimmed after the URL is set.

modified: src/pages/EditMonitor.vue
@homelab-alpha homelab-alpha marked this pull request as ready for review December 31, 2024 06:10
@homelab-alpha
Copy link
Contributor Author

Before merging this PR into the master branch, we first need to decide what to do with PR #5438, as it conflicts with my PR. For more details, please refer to the PR description above.

My preference is to close PR #5438 and merge this PR into the main branch.

Additionally, I have added @Ionys320, to the Co-authored-by section for FR #5422, as we worked on it together.

@CommanderStorm, What is your opinion on this?

@Ionys320
Copy link
Contributor

Ionys320 commented Jan 7, 2025

Hi,
My issue #5466 is independant of any PR, in theory. The problem exists because of my PR #5438 because it uses more fields than before, but even so, the fields are not cleaned when switching monitor type. That's the issue.

Regarding regex search, my PR #5438 is supposed to do the work. So the only thing this PR should do would be cleaning the fields, if this is mendated. That's why I didn't opened any other PR after creating my issue. We need an approval before adding a fields cleaning processs.

@homelab-alpha
Copy link
Contributor Author

Hey @Ionys320,

If you've read the PR carefully, I have included the feature request for #5422 in this PR, along with the solution to the problem you had with your own PR #5438. The transition of searchText from stringbase to regexbase is now complete and working properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability Seach Monotor List via Hostnames or URL Original fields are making search noise
2 participants