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

Feature: Clone existing monitor #2489

Merged
merged 9 commits into from
Feb 25, 2023
+67 −6
Merged

Feature: Clone existing monitor #2489

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
608e3f5
Feature: Clone existing monitor
mathiash98 Dec 27, 2022
4d0bdae
Add jsdoc for Tag type
mathiash98 Dec 27, 2022
239910a
Change clone icon from plus to clone
mathiash98 Jan 2, 2023
f0ae67f
Merge remote-tracking branch 'origin/master' into mathiash98/clone-mo…
louislam Feb 25, 2023
54cd7a0
Merge manually
louislam Feb 25, 2023
4a5a424
Merge remote-tracking branch 'origin/master' into mathiash98/clone-mo…
louislam Feb 25, 2023
f9a6d7e
Add a missing icon
louislam Feb 25, 2023
43c797a
Do not active the old monitor in the clone page
louislam Feb 25, 2023
4fed0c1
Show `Copy of` in front of the cloned monitor name
louislam Feb 25, 2023
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
10 changes: 8 additions & 2 deletions src/components/Tag.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,15 @@
</template>

<script>
/**
* @typedef {import('./TagsManager.vue').Tag} Tag
*/

export default {
props: {
/** Object representing tag */
/** Object representing tag
* @type {Tag}
*/
item: {
type: Object,
required: true,
Expand All @@ -32,7 +38,7 @@ export default {
},
/**
* Size of tag
* @values normal, small
* @type {"normal" | "small"}
*/
size: {
type: String,
Expand Down
20 changes: 19 additions & 1 deletion src/components/TagsManager.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,24 +134,42 @@ import { colorOptions } from "../util-frontend";
import Tag from "../components/Tag.vue";
const toast = useToast();

/**
* @typedef Tag
* @type {object}
* @property {number | undefined} id
* @property {number | undefined} monitor_id
* @property {number | undefined} tag_id
* @property {string} value
* @property {string} name
* @property {string} color
* @property {boolean | undefined} new
*/

export default {
components: {
Tag,
VueMultiselect,
},
props: {
/** Array of tags to be pre-selected */
/** Array of tags to be pre-selected
* @type {Tag[]}
*/
preSelectedTags: {
type: Array,
default: () => [],
},
},
data() {
return {
/** @type {Modal | null} */
modal: null,
/** @type {Tag[]} */
existingTags: [],
processing: false,
/** @type {Tag[]} */
newTags: [],
/** @type {Tag[]} */
deleteTags: [],
newDraftTag: {
name: null,
Expand Down
5 changes: 5 additions & 0 deletions src/icon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import { FontAwesomeIcon } from "@fortawesome/vue-fontawesome";

// Add Free Font Awesome Icons
// https://fontawesome.com/v5.15/icons?d=gallery&p=2&s=solid&m=free
// In order to add an icon, you have to:
// 1) add the icon name in the import statement below;
// 2) add the icon name to the library.add() statement below.
import {
faArrowAltCircleUp,
faCog,
Expand Down Expand Up @@ -45,6 +48,7 @@ import {
faHeartbeat,
faFilter,
faInfoCircle,
faClone,
} from "@fortawesome/free-solid-svg-icons";

library.add(
Expand Down Expand Up @@ -90,6 +94,7 @@ library.add(
faHeartbeat,
faFilter,
faInfoCircle,
faClone,
);

export { FontAwesomeIcon };
Expand Down
2 changes: 2 additions & 0 deletions src/lang/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,8 @@
"uninstalling": "Uninstalling",
"confirmUninstallPlugin": "Are you sure want to uninstall this plugin?",
"notificationRegional": "Regional",
"Clone Monitor": "Clone Monitor",
"Clone": "Clone",
"smtp": "Email (SMTP)",
"secureOptionNone": "None / STARTTLS (25, 587)",
"secureOptionTLS": "TLS (465)",
Expand Down
3 changes: 3 additions & 0 deletions src/pages/Details.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
<router-link :to=" '/edit/' + monitor.id " class="btn btn-normal">
<font-awesome-icon icon="edit" /> {{ $t("Edit") }}
</router-link>
<router-link :to=" '/clone/' + monitor.id " class="btn btn-normal">
<font-awesome-icon icon="clone" /> {{ $t("Clone") }}
</router-link>
<button class="btn btn-danger" @click="deleteDialog">
<font-awesome-icon icon="trash" /> {{ $t("Delete") }}
</button>
Expand Down
27 changes: 24 additions & 3 deletions src/pages/EditMonitor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,13 +682,23 @@ export default {
},

pageName() {
return this.$t((this.isAdd) ? "Add New Monitor" : "Edit");
let name = "Add New Monitor";
if (this.isClone) {
name = "Clone Monitor";
} else if (this.isEdit) {
name = "Edit";
}
return this.$t(name);
Comment on lines +685 to +691
Copy link
Contributor

@shynekomaid shynekomaid Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

},

isAdd() {
return this.$route.path === "/add";
},

isClone() {
return this.$route.path.startsWith("/clone");
},

isEdit() {
return this.$route.path.startsWith("/edit");
},
Expand Down Expand Up @@ -906,11 +916,22 @@ message HealthCheckResponse {
this.monitor.notificationIDList[this.$root.notificationList[i].id] = true;
}
}
} else if (this.isEdit) {
} else if (this.isEdit || this.isClone) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would disagree

this.$root.getSocket().emit("getMonitor", this.$route.params.id, (res) => {
if (res.ok) {
this.monitor = res.monitor;

if (this.isClone) {
/**
* Cloning a monitor will include properties that can not be posted to backend
* as they are not valid columns in the SQLite table.
*/
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
Copy link
Owner

Choose a reason for hiding this comment

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

Noticed that, maybe fix it in another pr.

}

// Handling for monitors that are created before 1.7.0
if (this.monitor.retryInterval === 0) {
this.monitor.retryInterval = this.monitor.interval;
Expand Down Expand Up @@ -981,7 +1002,7 @@ message HealthCheckResponse {
this.monitor.url = this.monitor.url.trim();
}

if (this.isAdd) {
if (this.isAdd || this.isClone) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@shynekomaid shynekomaid Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this.$root.add(this.monitor, async (res) => {

if (res.ok) {
Expand Down
4 changes: 4 additions & 0 deletions src/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ const routes = [
},
],
},
{
path: "/clone/:id",
component: EditMonitor,
},
{
path: "/add",
component: EditMonitor,
Expand Down