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

fix: Keep the change to the "Show Clickable Link" checkbox until entire dashboard is saved #5250

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

Conversation

R-HNF
Copy link
Contributor

@R-HNF R-HNF commented Oct 26, 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

Send url always because it's used in showLink() and fix the isClicable status.

Additional small changes:

  • Fix variable nameisClickAble -> isClickable.
  • Add attributes to the definition of monitor in data().

I confirmed my fix by checking and unchecking the checkbox, and opening and closing the dialog with the following log output.

console.info({
    isClickable: this.showLink(monitor),
});

Fixes #3794

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

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.

@R-HNF R-HNF changed the title Keep the change to the “Show Clickable Link" checkbox until entire dashboard is saved Keep the change to the "Show Clickable Link" checkbox until entire dashboard is saved Oct 26, 2024
@R-HNF R-HNF marked this pull request as ready for review October 26, 2024 09:08
@R-HNF R-HNF changed the title Keep the change to the "Show Clickable Link" checkbox until entire dashboard is saved fix: Keep the change to the "Show Clickable Link" checkbox until entire dashboard is saved Oct 26, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I have attached two comments below which seem somewhat suspicious

src/components/MonitorSettingDialog.vue Outdated Show resolved Hide resolved
src/components/MonitorSettingDialog.vue Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Oct 26, 2024
if (this.$parent.editMode && ignoreSendUrl && Object.keys(this.$root.monitorList).length) {
return this.$root.monitorList[monitor.element.id].type === "http" || this.$root.monitorList[monitor.element.id].type === "keyword" || this.$root.monitorList[monitor.element.id].type === "json-query";
}
return monitor.element.sendUrl && monitor.element.url && monitor.element.url !== "https://" && !this.editMode;
Copy link
Contributor Author

@R-HNF R-HNF Oct 28, 2024

Choose a reason for hiding this comment

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

remove this.editMode because it's undefined.

@@ -48,14 +48,11 @@ class Monitor extends BeanModel {
let obj = {
id: this.id,
name: this.name,
sendUrl: this.sendUrl,
sendUrl: !!this.sendUrl,
url: this.url,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you move url up here?

There are cases where we don't want to expose this information
=> currently, this would leak that as far as I can see..

Copy link
Contributor Author

@R-HNF R-HNF Oct 29, 2024

Choose a reason for hiding this comment

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

@CommanderStorm
It's used in showLink(). Should I have not deleted the following after all?

if (this.sendUrl) {
obj.url = this.url;
}

Leaving this if-statement causes the issue described in #3794 because when sendUrl is false, the url isn’t sent from database, and the result of showLink() also becomes false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Show Clickable Link" checkbox does not update until entire dashboard is saved
2 participants