Calculation Endpoints: Adds Validation and Tests #1521

agosmou · 2023-11-17T08:09:41Z

Fixes #1499

Sub-Issue from #1238

What changes did you make?

Added integration tests for the 'calculation' endpoints that test the functional requirements of the calculation workflow as well as edge cases
No need for validation as overexposed endpoints were deleted

Why did you make the changes (we will use this info to test)?

Calculation Workflow Quality: Added tests for 'calculation' endpoints to ensure workflow meets functional requirements and handles edge cases
Enhanced CI/CD: Integration tests in TDM CI pipeline prevent PRs that merge breaking changes
Reliable Testing: Integrated 'Jest' and 'testcontainers' to add robust testing reflecting our production setup

Notes

See Test Wiki for testing documentation on TDM Calculator

agosmou · 2023-11-17T08:11:31Z

server/.env.example

I changed the file name from
.example.env -> .env.example as it was before (better naming)

agosmou · 2023-11-17T08:13:05Z

server/.env.example

-# EMAIL_SENDER=
-# EMAIL_PUBLIC_COMMENT_LA_CITY=
-# EMAIL_PUBLIC_COMMENT_WEB_TEAM=
+# SENDGRID_API_KEY=SG.testAPIkey


Since the testing environment is local and isolated, I added the dummy variables with fake API keys into the example .env file since we already have them in the CI file .github/workflows/tdm-server-tests.yml

Does this mean tdm-server-tests can run with .env.example?

Nope! I just changed it because VSCode has better highlighting with the new name. Also included the variables so new members could run the tests easily without the formal variables. I tried to get this to work unsuccessfully with ./push.sh
An alternate strategy I thought of this morning is a function in the push script that can uncomment the test variables for the server tests and then re-comment upon completion. I'll have to check if this is viable, but editing the .env file with code feels unkosher. Thoughts?

I wouldn't edit the .env file. Instead, switch between files. That way the server tests can have one. Local dev can have another, prod, etc.

https://stackoverflow.com/questions/55406055/toggle-between-multiple-env-files-like-env-development-with-node-js

I was looking into that but had some concerns.

Since the container has a single database that is restored before every test suite (i.e. before every test file), the logic that handles this will call tedious-pool.js the connection logic from the server which will consume the original .env file. this means that this file would require a conditional to see which .env to consume right? Is this crufty?

It means everywhere we use dotenv should probably go through a tdmenv wrapper.

so new issue for refactoring codebase with this ^

agosmou · 2023-11-17T08:14:02Z

server/app/services/calculation.service.js

@@ -38,7 +38,7 @@ const post = async item => {
    request.output("id", mssql.Int, null);
    const response = await request.execute("Calculation_Insert");

-    return response.outputParameters;
+    return response.output;


This was returning an undefined variable so I used the VSCode debugger to check what the property was supposed to be - it's been rectified now and actually returns the id

agosmou · 2023-11-17T08:14:48Z

server/__tests__/calculation.test.js

+  });
+
+  // PUT "/:id" Update calculation using inexistent id (Admin only)
+  //TODO: this endpoint logic needs error handling for inexistent ids


Consider adding issue - inexistent id's also returned 200 status codes

agosmou · 2023-11-17T08:14:53Z

server/__tests__/calculation.test.js

+  });
+
+  // DELETE "/:id" Delete a calculation using inexistent id (Admin only)
+  //TODO: this endpoint needs error handling for inexistent ids


Consider adding issue - inexistent id's also returned 200 status codes

ZekeAranyLucas

Looking good. What prompted adding the schema validation?

server/app/controllers/calculation.controller.js

ZekeAranyLucas · 2023-11-17T15:36:03Z

server/__tests__/calculation.test.js

+  });
+
+  // DELETE "/:id" Delete a calculation using inexistent id (Admin only)
+  //TODO: this endpoint needs error handling for inexistent ids


ZekeAranyLucas · 2023-11-17T15:36:12Z

server/__tests__/calculation.test.js

+  });
+
+  // PUT "/:id" Update calculation using inexistent id (Admin only)
+  //TODO: this endpoint logic needs error handling for inexistent ids


ZekeAranyLucas · 2023-11-17T15:38:11Z

server/.env.example

-# EMAIL_SENDER=
-# EMAIL_PUBLIC_COMMENT_LA_CITY=
-# EMAIL_PUBLIC_COMMENT_WEB_TEAM=
+# SENDGRID_API_KEY=SG.testAPIkey


Does this mean tdm-server-tests can run with .env.example?

ZekeAranyLucas · 2023-11-17T15:53:07Z

server/app/schemas/calculation.post.js

+    name: {
+      type: "string",
+      minLength: 1,
+      pattern: "^[a-zA-Z '.-]+$"


What will happen in the UX if the user breaks validation?

I think the create, update , delete, calculation endpoints are not used by the client currently - future implementation. I think we mostly use the GET endpoints. Also, there is only '1' calculation in the db currently.

Then how do you know if this validation is right?

The answer could be you are making stubs, but then the code needs a comment that the regex are not spec'ed.

I am looking for answers to why in the code.

And unused endpoints are risky. I am inclined to delete dead code as a security risk.

Is there a plan for when to add it?

excellent question. Ill have to refer to @entrotech

not to mention validation logic may even require numbers to be valid (not included currently)

server/__tests__/calculation.test.js

agosmou · 2023-11-17T18:14:58Z

Looking good. What prompted adding the schema validation?

@ZekeAranyLucas

Request from John to add validation for all web api endpoints - #1238

ZekeAranyLucas

In the name of security, I recommend deleting unused service methods. This change becomes much smaller. 😀

ZekeAranyLucas · 2023-11-17T19:00:28Z

In the name of security, I recommend deleting unused service methods. This change becomes much smaller. 😀

After looking at #1238, I have better context. Probably better to Write tests for the faq crud APIs first. Those ones are accessible in the client, and need the attention.

ZekeAranyLucas · 2023-11-17T19:23:37Z

Ok, editing calculations is covered by issuer #46. Given the nature of how rules work, I suggest only devs should ever change rules. They are too finicky for normal users to get right. Allowing an admin to delete a rule is a recipe for disaster

I recommend:

resolve 46 as "won't fix"
delete related service methods
use Implement Server-Side Request Validation and API endpoint tests #1238 issue to identify similar overexposed public surface and delete.

What do you think @entrotech ?

ZekeAranyLucas · 2023-11-21T00:39:50Z

@agosmou is this ready to roll? are you going to create issues after merging?

entrotech · 2023-11-21T01:45:24Z

@ZekeAranyLucas comments above are spot-on. Early on, we had thoughts of implemented admin screens for versioning the calculations, allowing a super-admin to edit rules on an existing calculation to create a new version, and apply that version to future projects, but the implications are kinda scary, and I like having enough friction in changing rules that they are made mostly by DB Migrations and an occasional code change, so they need to go through a QA process by people that know what they are doing.

agosmou · 2023-11-21T18:50:51Z

@ZekeAranyLucas @entrotech
Just to get proper sign-off before going and deleting services,
To confirm, the below are the action items for this issue to be merged:

resolve 46 as "won't fix"
delete related service methods
- calculation in 'server/app/services/calculation.service.js'
  - post
  - put
  - delete
- calculation in 'client/src/services/calculation.service.js'
  - post(calculation)
  - put(calculation)
  - del(id)
use Implement Server-Side Request Validation and API endpoint tests #1238 isssue to identify similar overexposed public surface and delete

agosmou · 2023-11-21T20:27:06Z

The 'rule service' uses the getById service from the 'calculations' server service so this will remain for now.

agosmou · 2023-11-21T20:41:18Z

The above requested changes were addressed in the latest commit f1dcd6d

entrotech

I trust you two to have worked out the details on this PR. I am agreeing on deleting the endpoints you mentioned.

BTW, there is a remnant of Issue #46 that I can't bring myself to remove, just cause it's kinda cool. Log in as an admin and change the client path to /admin and it give you an interesting view of the rules that make up the calculation.

@agosmou you can merge this yourself, if you want.

ZekeAranyLucas · 2023-11-22T01:25:24Z

/admin is cool! i wouldn't remove that.

agosmou · 2023-11-22T01:51:23Z

Yes! Very cool (see below for record). This is unaffected by this PR.

agosmou added 5 commits November 16, 2023 23:50

adds calculation tests for server

477aed8

adds admin variables to CI

3294ba2

adds validation for request bodies for calculation endpoints

b00f932

linting on test file

a7d28bf

Fix: resolved undefined return to return id property

2a85e69

agosmou added role: back-end Node/Express Development Task priority: SHOULD HAVE feature: Testing/Error feature: DevOps labels Nov 17, 2023

agosmou added this to the 02 - Security milestone Nov 17, 2023

agosmou self-assigned this Nov 17, 2023

agosmou commented Nov 17, 2023

View reviewed changes

server/.env.example

Copy link

Member Author

agosmou Nov 17, 2023

Choose a reason for hiding this comment

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

I changed the file name from
.example.env -> .env.example as it was before (better naming)

agosmou commented Nov 17, 2023

View reviewed changes

agosmou requested review from entrotech and ZekeAranyLucas November 17, 2023 08:22

ZekeAranyLucas suggested changes Nov 17, 2023

View reviewed changes

adds tests with invalid bodies

c153a88

fixes test description for invalid bodies

94e9032

ZekeAranyLucas suggested changes Nov 17, 2023

View reviewed changes

refactors tests

7f1deff

ZekeAranyLucas approved these changes Nov 21, 2023

View reviewed changes

removes stale endpoints

f1dcd6d

agosmou mentioned this pull request Nov 21, 2023

An Admin should be able to create and maintain calculations #46

Closed

6 tasks

entrotech approved these changes Nov 22, 2023

View reviewed changes

agosmou merged commit 917c6d4 into develop Nov 22, 2023
1 check passed

agosmou deleted the 1499-ag-calculation-tests-and-validation branch November 22, 2023 01:56

agosmou mentioned this pull request Nov 25, 2023

Refactors Account API Endpoints Tests #1535

Merged

Calculation Endpoints: Adds Validation and Tests #1521

Calculation Endpoints: Adds Validation and Tests #1521

Conversation

agosmou commented Nov 17, 2023 • edited Loading

What changes did you make?

Why did you make the changes (we will use this info to test)?

Notes

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZekeAranyLucas left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agosmou commented Nov 17, 2023

ZekeAranyLucas left a comment

Choose a reason for hiding this comment

ZekeAranyLucas commented Nov 17, 2023 • edited Loading

ZekeAranyLucas commented Nov 17, 2023

ZekeAranyLucas commented Nov 21, 2023

entrotech commented Nov 21, 2023

agosmou commented Nov 21, 2023 • edited Loading

agosmou commented Nov 21, 2023

agosmou commented Nov 21, 2023

entrotech left a comment

Choose a reason for hiding this comment

ZekeAranyLucas commented Nov 22, 2023

agosmou commented Nov 22, 2023 • edited Loading

agosmou commented Nov 17, 2023 •

edited

Loading

ZekeAranyLucas commented Nov 17, 2023 •

edited

Loading

agosmou commented Nov 21, 2023 •

edited

Loading

agosmou commented Nov 22, 2023 •

edited

Loading