web/multinode: Show appropriate error while adding new node

[Pranav2612000]

While adding new nodes, if the nodeID, Public IP Address, or
the API Key is incorrect no error was being shown on the webpage.
This commit resolves this issue by showing an error notification on
the top right corner of the screen with appropriate error text.

Fixes #4199

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

• NEW: Are there any Satellite database migrations? Are they forwards and backwards compatible?
• Does the PR describe what changes are being made?
• Does the PR describe why the changes are being made?
• Does the code follow our style guide?
• Does the code follow our testing guide?
• Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
• Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
• Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
• Does any documentation need updating?
• Do the database access patterns make sense?

[Pranav2612000]

Hey! This is my first PR to this project so let me know if anything is missing! Thank you.

[mobyvb]

Hey @Pranav2612000, thank you for your pull request :)

Before I review this change, could you get the build to pass? It looks like there are currently some linting errors:
https://build.dev.storj.io/blue/organizations/jenkins/storj/detail/PR-4293/1/pipeline/
By the way, you can see the outcome of future builds by clicking "Details" next to "continuous-integration/jenkins/pr-head — This commit cannot be built" at the bottom of your PR

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Pranav2612000 on file. Once you have signed the CLA, please let us know, so we can manually review and add you to the approved contributors list.

[mobyvb]

@Pranav2612000 , one more thing: could you upgrade your npm version to at least version 7? For reference, my npm/node versions are 7.5.1/15.8.0 (you shouldn't need those exact versions). But we think part of the reason why so many lines were removed from your package-lock.json is that you have an older version of npm.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Pranav2612000 on file. Once you have signed the CLA, please let us know, so we can manually review and add you to the approved contributors list.

[Pranav2612000]

Hey @mobyvb ! I've updated nvm and node to 7.5.1/15.8.0 as you asked.

[stefanbenten]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[egonelbre]

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

[egonelbre]

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

[mobyvb]

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).

[mobyvb]

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?

[egonelbre]

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.

[mobyvb]

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.

[Pranav2612000]

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 )

[profclems]

@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 ?

[storjrobot]

This pull request has been mentioned on Storj Community Forum (official). There might be relevant details there:

https://forum.storj.io/t/multinode-seems-to-stop-where-it-should-continue/21697/14