Fiqare perseo-fe improvements by fiqare-emergya-dev · Pull Request #419 · telefonicaid/perseo-fe

fiqare-emergya-dev · 2019-12-16T16:42:09Z

This pull request contains the improvements made by the Emergya team for perseo-fe. These improvements are part of the fiQare project, which is based on ISO 25010 to improve software quality. More info: https://fiqare.eu/

Changes related to variable changes have been removed in this PR.

Swagger is provided for HTTP protocol in:
<server_host>:9090/api-docs

Note: npm install is needed

Merge by Jenkins

…js lib/models/entitiesStore.js lib/models/paths.js lib/models/postAction.js lib/models/rules.js lib/models/visualRules.js lib/myutils.js

Merge by Jenkins

…d/master

Merge by Jenkins

fgalan · 2019-12-18T17:24:31Z

lib/routes/visualRulesController.js

            return myutils.respond(resp, err, data);
        }
        if (data && data.name) {
-            //resp.location(req.url.replace(new RegExp(req.params.id), '')+encodeURIComponent(data.name));


@AlvaroVega maybe this is an old leftover? It's a good idea to remove it?

I guess it could be deleted

Thus, change is fine. NTC.

fgalan · 2019-12-18T17:29:19Z

documentation/eslint.md

@@ -0,0 +1,74 @@
+# Development documentation (Third Iteration)


Not sure of understanding the purpose of this file...

Apart from the title (what is "third iteration"? maybe it makes sense in the context of fiQare project, but not this repo is project-agnostic), this seem to be just a description of eslint based on standard content.

I mean, I could understand a replacmente of jslint with eslint, as for instance is done by PR telefonicaid/iotagent-ul#408 in the case of IOTA-UL (although not in the scope of this PR but in a new one). However just including a piece of documentation about eslint in the repository doesn't have too much sense to me.

Maybe I'm not getting the point or something. Could you elaborate on this? If it is easier for you, you can tell me by email.

This is a style guide made for the Fiqare project, if it is not useful for Perseo Front or for future developers we remove it from the PR.

Yes, it seems to be too much fiQare-specific... Removing it from the PR sounds good to me.

Fixed in ce583be

fgalan · 2020-01-07T19:51:05Z

At the present moment (January 7th, 2019), there are some comment threads (at least three) not yet answered/fixed so I understand the work on this PR is ongoing.

@fiqare-emergya-dev, is my understanding correct?

fiqare-emergya-dev · 2020-01-10T10:08:12Z

I've already reviewed the comment threads.

fgalan · 2020-01-14T12:34:51Z

lib/models/emailAction.js

-var transporter = nodemailer.createTransport(smtpTransport(config.smtp));
+var transporter = nodemailer.createTransport(config.smtp);


Why this change (along with the removal in L28) is actually needed?

The function createTransport accepts the configuration as a string, so the deleted var in L28 was no necessary and redundant.

fgalan · 2020-01-14T12:35:36Z

lib/models/emailAction.js

        metrics.IncMetrics(event.service, event.subservice, metrics.actionEmail);

-        transporter.sendMail(mailOptions, function(err, info) {
+        transporter.sendMail(mailOptions, (err, info) => {


I understand the preferred code style is currently function(error,info) over (err, info) =>, so this change probably should be rolled-back.

Fixed in 9c80eb9

fgalan · 2020-01-14T12:36:28Z

lib/models/rules.js

    MaxNameLength = 50,
-    errors = {};
+    errors = {},
+    logger = require('logops');


Better to have all the require() together. Pleace move up (just afeter actions declaration).

Fixed in 9c80eb9

fgalan · 2020-01-14T12:37:20Z

lib/perseo.js

+                swagger: '2.0', // Specification (optional, defaults to swagger: '2.0')
+                info: {
+                    title: 'Perseo Front-End', // Title (required)
+                    version: '1.7.0-fiqare' // Version (required)


This should be got from packages.json

Fixed in 9c80eb9

fgalan · 2020-01-14T12:44:28Z

package.json

    "nodemailer-smtp-transport": "~0.1.13",
    "request": "2.88.0",
+    "smpp": "0.3.1",
+    "swagger-jsdoc": "~3.4.0",


documentation/development.md documentation should be modified to include a section about swagger (similar to what it has been done i n telefonicaid/iotagent-ul#412)

readme.md updated in 9c80eb9

fgalan · 2020-01-14T12:50:19Z

I have done a new review round. Please, find my new comments in the PR.

In addition to them, a couple of additional comments:

It would be great if you could squash & rebase your PR to pass from 107 commits to 1 commit (or a small number)
Review Travis CI results, as it seems the PR is not passing the tests

Thanks!

fgalan · 2020-01-30T09:07:56Z

README.md

 > does not need to be released as under the same license, or even released as open source.
+
+
+## User & Programmers Manual


I think README.md file is not the right place to include this. In addition, it should be simplified: you have already included the needed packages in packages.json and so on, so please focus just in the usage.

In sum, It should be included in development.md, just after the last item (Prettify Code) and be like this:

### Swagger In order to run Swagger, you need to execute the Perseo FE (as explained [here](deployment.md) and then you can access to:

<server_host>:9090/api-docs

The swagger documentation provided at /api-docs covers all the HTTP endpoint exposed by Perseo FE.

Fixed in 210813b

fgalan · 2020-01-30T09:09:13Z

lib/models/rulesStore.js

 function findAll(service, subservice, callback) {
    var criterion = {};
-
+    


Remove this extra whitespace, pls.

Fixed in 210813b

fgalan · 2020-01-30T09:10:05Z

lib/perseo.js

+    d = domain.create(),
+    swaggerJsdoc = require('swagger-jsdoc'),
+    swaggerUi = require('swagger-ui-express'),
+    pjson = require('./package.json');


Suggested change

pjson = require('./package.json');

pjson = require('../package.json');

Not sure, please check.

it is correct

Ok. Fixed in df81a41

fgalan · 2020-01-30T09:12:26Z

lib/alarm.js

 function raise(alarm, context, message) {
    var state = alarms[alarm];
    context = (process.domain && process.domain.context) || {};
+


CHANGES_NEXT_RELEASE entries should be added regarding the changes in this documentation. Suggestion:

- Add: /api-docs endpoint providing swagger-based documentation of the HTTP endpoints exposed by Perseo FE - Hardening: software quality improvement based on ISO25010 recommendations

Fixed in 210813b

AlvaroVega · 2020-02-05T17:24:06Z

test/component/metrics/metrics_actions_test.js

                                should.equal(m.services.unknownt.sum.actionEntityUpdate, 1);
                                should.equal(m.services.unknownt.sum.okActionEntityUpdate, 0);
-                                should.equal(m.services.unknownt.sum.failedActionEntityUpdate, 1);
+                                should.equal(m.services.unknownt.sum.failedActionEntityUpdate, 0);


This is going to be handled in another PR: #425

In order not disturb @fiqare-emergya-dev (a conflict will happen when #425 gets merged), let's keep as it is now in this PR. I'll fix it in the prelanding branch (hardening/fiqare-perseo-fe-improvements-prelanding)

NTC (just informative)

fgalan · 2020-02-06T08:27:29Z

lib/models/updateAction.js

+            } else {
+                callback(err, data);
            }
-            callback(err, data);


I think this change is not correct... the callback() needs to be changed in any case and, after this change, it is only called when the execution flow goes by the "else" part.

Fixed in c29e89f

fgalan

LGTM. Thanks for your contribution and your effort along the several review rounds! :)

A PR will follow to this one for the definitive merge into master, after some minor adaptations.

fiqare-emergya-dev and others added 30 commits February 12, 2019 15:00

Fixed issue in lib/appContext.js

c620a0a

Fixed issues in lib/db.js

b1eb8fa

Fixed issue in lib/models/actions.js

d38ffcc

Fixed issues in lib/models/executionsStore.js

f18f17b

Fixed issue in lib/models/notices.js

fb9cb75

Fixed issue in lib/models/updateAction.js

d9d800d

Fixed issue in lib/models/perseo.js

e666027

Fixed issue in lib/models/actionsController.js

5f799c9

Merge pull request #1 from Emergya/284633

460c75f

Merge by Jenkins

Fixed issue #284635 in lib/models/actions.js lib/models/actionsStore.…

24bbb3f

…js lib/models/entitiesStore.js lib/models/paths.js lib/models/postAction.js lib/models/rules.js lib/models/visualRules.js lib/myutils.js

Merge pull request #2 from Emergya/284635

c87e96f

Merge by Jenkins

Fixed issue #284625 in lib/routes/actionsController.js

26a081e

Merge pull request #3 from Emergya/284625

9fdc5b9

Merge by Jenkins

Merge pull request #4 from Emergya/ci

8200b45

Merge by Jenkins

Merge branch 'telefonicaid-master'

15c1d5d

Merged from telefonicaid-perseo-fe:master

2e81912

Merge pull request #6 from Emergya/master

a514588

Merge by Jenkins

Merge pull request #7 from telefonicaid/master

b8b3ce2

Merge by Jenkins

Merge pull request #8 from Emergya/master

ca2b66c

Merge by Jenkins

Merged lib/db.js and lib/routes/actionsController.js from telefonicai…

ea6a05d

…d/master

Merge pull request #9 from Emergya/master

aa23fed

Merge by Jenkins

Merge pull request #10 from Emergya/ci

05ac1f4

Merge by Jenkins

Resolver issues Db.js #292343

88344c8

Resolver issues actions.js #292346

2e1025f

Resolve issues actionsStore.js #292348

e75d07d

Resolve issues executionsStore.js

2cd53b5

Resolver issues Metrics.js #292352

a3c1cc1

Resolver issues Notices.js #292353

c012cdc

Resolver issues Rules.js #292363

326a6b4

Resolver issues RulesStore.js #292365

9e873b8

fgalan reviewed Dec 18, 2019

View reviewed changes

fiqare-emergya-dev added 2 commits December 19, 2019 11:48

Fixed Swagger dependencies

51271cd

Revert ESLint docs

ce583be

Fixed indentation

d978818

fiqare-emergya-dev added 2 commits January 13, 2020 08:38

Fixed config.js

ff07d10

Fixed indentation

0865dc8

fgalan reviewed Jan 14, 2020

View reviewed changes

fiqare-emergya-dev and others added 2 commits January 27, 2020 10:19

Merge pull request #3 from telefonicaid/master

8371d53

PR fixed

9c80eb9

fgalan reviewed Jan 30, 2020

View reviewed changes

fiqare-emergya-dev added 2 commits February 3, 2020 10:39

travis fixed

df81a41

PR fixed

210813b

AlvaroVega reviewed Feb 5, 2020

View reviewed changes

fgalan reviewed Feb 6, 2020

View reviewed changes

fgalan changed the base branch from master to hardening/fiqare-perseo-fe-improvements-prelanding February 6, 2020 08:30

PR fixed

c29e89f

fgalan approved these changes Feb 6, 2020

View reviewed changes

fgalan merged commit 4877a73 into telefonicaid:hardening/fiqare-perseo-fe-improvements-prelanding Feb 6, 2020

fgalan mentioned this pull request Feb 6, 2020

FIX software quality improvement based on ISO25010 recommendations #428

Merged

		@@ -0,0 +1,74 @@
		# Development documentation (Third Iteration)

		var transporter = nodemailer.createTransport(smtpTransport(config.smtp));
		var transporter = nodemailer.createTransport(config.smtp);

		> does not need to be released as under the same license, or even released as open source.


		## User & Programmers Manual

		function findAll(service, subservice, callback) {
		var criterion = {};

	pjson = require('./package.json');
	pjson = require('../package.json');

Conversation

fiqare-emergya-dev commented Dec 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgalan Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgalan commented Jan 7, 2020

Uh oh!

fiqare-emergya-dev commented Jan 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgalan commented Jan 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgalan Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgalan Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgalan left a comment

Choose a reason for hiding this comment

Uh oh!

fgalan Dec 18, 2019 •

edited

Loading

fgalan Jan 30, 2020 •

edited

Loading

fgalan Feb 6, 2020 •

edited

Loading