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

Jump to bottom

web/multinode: Show appropriate error while adding new node #4293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+72 −6
Open

web/multinode: Show appropriate error while adding new node #4293

Show file tree
Hide file tree
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion web/multinode/.eslintrc.js
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@ module.exports = {
"vue/no-unused-refs": ["warn"],
"vue/no-useless-v-bind": ["warn"],

'vue/no-unregistered-components': ['warn', { ignorePatterns: ['router-link', 'router-view'] }],
'vue/no-unregistered-components': ['warn', { ignorePatterns: ['router-link', 'router-view', 'notifications'] }],

'storj/vue/require-annotation': 'warn',
},
Expand Down
43 changes: 39 additions & 4 deletions web/multinode/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions web/multinode/package.json
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@
"vue-chartjs": "3.5.1",
"vue-class-component": "7.2.6",
"vue-clipboard2": "0.3.1",
"vue-notification": "^1.3.20",
Copy link
Member

Choose a reason for hiding this comment

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

@mobyvb don't forget to review the dependency. https://github.com/storj/docs/blob/main/code/Dependencies.md

Copy link
Member

Choose a reason for hiding this comment

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

Also don't we already have some sort of notification system?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @egonelbre
I was planning on reviewing the dependency before approving.

We do have a custom notification system for the satellite web app. @VitaliiShpital has shared the following with me about how it is written/implemented:

ok, so it consists of explicit vuex module https://github.com/storj/storj/blob/main/web/satellite/src/store/modules/notifications.ts
notificator plugin https://github.com/storj/storj/blob/main/web/satellite/src/utils/plugins/notificator.ts
which is enabled in main.ts like this https://github.com/storj/storj/blob/main/web/satellite/src/main.ts#L21
here we add it to UI https://github.com/storj/storj/blob/main/web/satellite/src/App.vue#L8
and here’s an example of it’s usage https://github.com/storj/storj/blob/main/web/satellite/src/components/accessGrants/AccessGrants.vue#L119

I still need to audit the vue-notification package (link for me, later: https://github.com/euvl/vue-notification).

Copy link
Member

@mobyvb mobyvb Dec 3, 2021 •
edited

Choose a reason for hiding this comment

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

I have reviewed the vue-notification package, and it seems fine. It does not require any additional dependencies besides itself, and I didn't notice any issues in the code (there is not very much code).

@Pranav2612000 would you like me to review this PR as-is, or do you want to try implementing notifications like web/satellite, which I linked in my previous comment?

Copy link
Member

Choose a reason for hiding this comment

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

I have reviewed the vue-notification package, and it seems fine. It does not require any additional dependencies besides itself, and I didn't notice any issues in the code (there is not very much code).

That does not seem to be the case. There seem to be plenty of additions in the lock file, e.g. fsevents, node-notifier, fork-ts-checker-webpack-plugin-v5, vue-loader-v16, prettier, source-map. Did you compare the before and after of the node_modules folder?

And if we already have a system for notifications then we should diverge only if we have a really good reason and/or plan to move other implementations to it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I said that because I was looking at the package.json here https://github.com/euvl/vue-notification/blob/master/package.json and did not see any non-dev dependencies added.
But yes, I see that the package lock has been updated. And I have not audited any of those new packages other than vue-notification.

Copy link
Author

Choose a reason for hiding this comment

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

0kay. Does that mean we should use the notification system already being used? Can you point me to the documentation for it? I'll be happy to use it. ( I had also asked if we have a notification system before, but I got to know we don't have one and that's why I used this package )

Copy link
Member

Choose a reason for hiding this comment

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

@mobyvb the notification system we already have is only in the satellite so how do we share it with the web/storagenode and web/multinode ?

"vue-property-decorator": "9.1.2",
"vue-router": "3.4.9",
"vuex": "3.6.0"
Expand Down
10 changes: 10 additions & 0 deletions web/multinode/src/api/index.ts
View file
Open in desktop
Expand Up @@ -12,6 +12,15 @@ export class UnauthorizedError extends Error {
}
}

/**
* NotFound is a custom error type for page not found
*/
export class NotFoundError extends Error {
public constructor(message = 'not found') {
super(message);
}
}

/**
* BadRequestError is a custom error type for performing bad request.
*/
Expand Down Expand Up @@ -56,6 +65,7 @@ export class APIClient {

switch (response.status) {
case 401: throw new UnauthorizedError(body.error);
case 404: throw new NotFoundError(body.error);
case 400: throw new BadRequestError(body.error);
case 500:
default:
Expand Down
3 changes: 3 additions & 0 deletions web/multinode/src/app/App.vue
View file
Open in desktop
Expand Up @@ -4,12 +4,15 @@
<template>
<div id="app">
<router-view />
<notifications group="alerts" />
</div>
</template>

<script lang="ts">
import { Component, Vue } from 'vue-property-decorator';
import Notifications from 'vue-notification'

Vue.use(Notifications);
// @vue/component
@Component
export default class App extends Vue {
Expand Down
19 changes: 18 additions & 1 deletion web/multinode/src/app/views/AddFirstNode.vue
View file
Open in desktop
Expand Up @@ -48,6 +48,7 @@ import VButton from '@/app/components/common/VButton.vue';

import { Config as RouterConfig } from '@/app/router';
import { CreateNodeFields } from '@/nodes';
import { NotFoundError,BadRequestError } from '@/api';

// @vue/component
@Component({
Expand Down Expand Up @@ -99,12 +100,28 @@ export default class AddFirstNode extends Vue {

return;
}

try {
await this.$store.dispatch('nodes/add', this.nodeToAdd);
} catch (error) {
console.error(error.message);
this.isLoading = false;

if(error instanceof NotFoundError) {
this.$notify({
group: 'alerts',
type: 'error',
title: 'Error',
text: '404. Please check your Public IP Address'
});
} else if(error instanceof BadRequestError) {
this.$notify({
group: 'alerts',
type: 'error',
title: 'Error',
text: '400. Please check your NodeID and API Key'
});
}

}

await this.$router.push(RouterConfig.MyNodes.path);
Expand Down