Feature: Clone existing monitor #2489

mathiash98 · 2022-12-27T22:32:20Z

I have read and understand the pull request rules.

Description

Adds the feature of cloning existing monitor, I have briefly tested it with ping and https and ensured that all properties was cloned including notifications.

Flow:

Click on an existing monitor
Click clone -> User is redirected to /clone/ExistingMonitorId
Change fields
Click save -> User is redirected to /dashboard/NewMonitorId

Note: Tags will not be copied when cloning a monitor. The TagsManager component is a bit complicated and I did not want to spend that much more time on it.

Fixes #565
Fixes #2319

Type of change

Please delete any options that are not relevant.

User interface (UI)
New feature (non-breaking change which adds functionality)
Translation update
This change might require a documentation update

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 generate no new warnings

Demo

Uptime.Kuma.Clone.Feature.mov

Closes #565 Closes #2319 Adds the feature of cloning existing monitor, I have briefly tested it with ping and https and ensured that all properties was cloned including notifications.

Does not work properly but is still useful

src/pages/Details.vue

Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>

shynekomaid · 2023-01-19T09:13:31Z

src/pages/EditMonitor.vue

@@ -804,11 +814,22 @@ message HealthCheckResponse {
                        this.monitor.notificationIDList[this.$root.notificationList[i].id] = true;
                    }
                }
-            } else if (this.isEdit) {
+            } else if (this.isEdit || this.isClone) {


Perhaps this.isEdit, this.isClone should be separated for better readability, as there is a check inside this.isEdit for this.isClone. Yes, some code will be duplicated, but it will make it easier and more convenient to change the logic of this.isClone regardless of the logic of this.isEdit

I would disagree

shynekomaid · 2023-01-19T09:14:58Z

src/pages/EditMonitor.vue

@@ -866,7 +887,7 @@ message HealthCheckResponse {
                this.monitor.headers = JSON.stringify(JSON.parse(this.monitor.headers), null, 4);
            }

-            if (this.isAdd) {
+            if (this.isAdd || this.isClone) {


I would split it into two conditions so that it would be easier to make changes to either case in the future

And in general I would suggest a redo on the switchcases, it's not clear or obvious that the logic for editMonitor is in the other branch (in else scope) until you read the whole thing.

I would disagree. There is a lot of things that could be improved in this repo, but duplicating code does not make it better

Please make another PR for making it more clear that editMonitor is in the else bracket. Unrelated to this PR

shynekomaid · 2023-01-19T09:25:55Z

src/pages/EditMonitor.vue

+            let name = "Add New Monitor";
+            if (this.isClone) {
+                name = "Clone Monitor";
+            } else if (this.isEdit) {
+                name = "Edit";
+            }
+            return this.$t(name);


I would do as:

let name; if (this.isClone) { name = "Clone Monitor"; } else if (this.isAdd) { name = "Edit"; } else if (this.isEdit) { name = "Add New Monitor"; } else { name = "Wrong Monitor Action"; } return this.$t(name);

And I would add in en.js: Wrong Monitor Action

Also, it's not quite clear to me why the Edit Monitor translation key wouldn't be added, so that when you change the Edit key for other purposes, you don't risk making this one case incomprehensible

Mixed the name value for isAdd and isEdit?
Adding translations would be good yes, I just did as little as possible

shynekomaid

All in all good, but not without faults

mathiash98 · 2023-01-19T09:51:02Z

I will probably not do any more work on this, at least not the coming weeks. Please pick it up and do whatever changes you would like to do.

louislam · 2023-01-19T11:18:50Z

Just have a quick look. I think @mathiash98's code is reasonable.

@tminei's opinions are good too, but sometimes rewriting it requires to test everything again. It may be not worth the time.

So as long as it is working, I will still merge it. The coding practice is not very important in this case.

I have planned to merge this pr in 1.21.0, I will come back later.

…nitor # Conflicts: # src/languages/en.js # src/pages/EditMonitor.vue

…nitor

louislam · 2023-02-25T08:56:28Z

src/pages/EditMonitor.vue

+                            this.monitor.id = undefined; // Remove id when cloning as we want a new id
+                            this.monitor.includeSensitiveData = undefined;
+                            this.monitor.maintenance = undefined;
+                            this.monitor.tags = undefined; // FIXME: Cloning tags does not work yet


Noticed that, maybe fix it in another pr.

shynekomaid · 2023-02-27T07:17:30Z

.dockerignore

@@ -33,7 +33,7 @@ tsconfig.json
 /ecosystem.config.js
 /extra/healthcheck.exe
 /extra/healthcheck
-
+extra/exe-builder


why not "/extra/exe-builder" like aboves?

The pattern doc/frotz and /doc/frotz have the same effect in any .gitignore file. In other words, a leading slash is not relevant if there is already a middle slash in the pattern.

So it does not matter, but nice to have for following style

Style is relevant

mathiash98 added 2 commits December 27, 2022 23:26

Feature: Clone existing monitor

608e3f5

Closes #565 Closes #2319 Adds the feature of cloning existing monitor, I have briefly tested it with ping and https and ensured that all properties was cloned including notifications.

Add jsdoc for Tag type

4d0bdae

Does not work properly but is still useful

Saibamen suggested changes Jan 2, 2023

View reviewed changes

src/pages/Details.vue Outdated Show resolved Hide resolved

Change clone icon from plus to clone

239910a

Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>

louislam added this to the 1.21.0 milestone Jan 3, 2023

shynekomaid reviewed Jan 19, 2023

View reviewed changes

louislam added 5 commits February 25, 2023 14:56

Merge remote-tracking branch 'origin/master' into mathiash98/clone-mo…

f0ae67f

…nitor # Conflicts: # src/languages/en.js # src/pages/EditMonitor.vue

Merge manually

54cd7a0

Merge remote-tracking branch 'origin/master' into mathiash98/clone-mo…

4a5a424

…nitor

Add a missing icon

f9a6d7e

Do not active the old monitor in the clone page

43c797a

louislam reviewed Feb 25, 2023

View reviewed changes

Show Copy of in front of the cloned monitor name

4fed0c1

louislam merged commit 487eae7 into louislam:master Feb 25, 2023

mathiash98 deleted the mathiash98/clone-monitor branch February 26, 2023 22:25

shynekomaid reviewed Feb 27, 2023

View reviewed changes

chakflying mentioned this pull request Mar 24, 2023

Fix: assign tags when cloning monitor #2869

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Clone existing monitor #2489

Feature: Clone existing monitor #2489

mathiash98 commented Dec 27, 2022 •

edited

Loading

shynekomaid Jan 19, 2023

mathiash98 Jan 19, 2023

shynekomaid Jan 19, 2023

shynekomaid Jan 19, 2023 •

edited

Loading

mathiash98 Jan 19, 2023

mathiash98 Jan 19, 2023

shynekomaid Jan 19, 2023 •

edited

Loading

mathiash98 Jan 19, 2023

shynekomaid left a comment

mathiash98 commented Jan 19, 2023

louislam commented Jan 19, 2023

louislam Feb 25, 2023

shynekomaid Feb 27, 2023

mathiash98 Feb 27, 2023

shynekomaid Feb 27, 2023

Feature: Clone existing monitor #2489

Feature: Clone existing monitor #2489

Conversation

mathiash98 commented Dec 27, 2022 • edited Loading

Description

Flow:

Type of change

Checklist

Demo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shynekomaid Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shynekomaid Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shynekomaid left a comment

Choose a reason for hiding this comment

mathiash98 commented Jan 19, 2023

louislam commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiash98 commented Dec 27, 2022 •

edited

Loading

shynekomaid Jan 19, 2023 •

edited

Loading

shynekomaid Jan 19, 2023 •

edited

Loading