Fix failing tests

[jazhen]

• Add check to throw if found id collisions + test
• Stopped renaming datePlanted as planted since datePlanted is the name of the table column and planted is a key returned by the vector tiles
• Uncomment expressErrorHandler to regain central error handler functionality
• Add --experimental-vm-modules flag to launch.json so that the Jest supports ESModules in the VS Cdde debugger.
• Remove unused and commented code
  • bodyParser.json() is made redundant by express.json()
  • deleted variables in trees-router.js that we not being used

[zoobot]

Thanks for fixing the tests!! Can you add more description of what's happening in this commit?
What does that vm do in launch.json?
Is there a native fetch version of that axiosAPI test?
I'd previously commented out the index's error handler because it was returning too generic an error or breaking the sql when it errored. Is that fixed with this commit?

[jazhen]

What does that vm do in launch.json?

The --experimental-vm-modules flag makes it so that Jest can support ESM projects.

Is there a native fetch version of that axiosAPI test?

I don't know if I fully understand this question, but we can eventually make a function that uses native fetch to replicate the axios call. That would remove the need for axios as an external dependency.

I'd previously commented out the index's error handler because it was returning too generic an error or breaking the sql when it errored. Is that fixed with this commit?

Was the change solely for debugging purposes? If so I can add a quick change so that we log all SQL calls. Also, would like to improve our logging overall. Otherwise I tested the changes (creating and getting a tree) and things seemed fine on my end.

[zoobot]

we can eventually make a function that uses native fetch to replicate the axios call. That would remove the need for axios as an external dependency.

Sounds good!

I'd previously commented out the index's error handler because it was returning too generic an error or breaking the sql when it errored. Is that fixed with this commit?

Was the change solely for debugging purposes? If so I can add a quick change so that we log all SQL calls. Also, would like to improve our logging overall. Otherwise I tested the changes (creating and getting a tree) and things seemed fine on my end.

I think when sql returned bad data or something, it would break and not return anything or send back a response other than a 500.

[jazhen]

I think when sql returned bad data or something, it would break and not return anything or send back a response other than a 500.

Right I remember now. I think we have to talk about what counts as an expected result unexpected result. In my mind if the sql call is formatted wrong, then we should return a 500 error. But in this case for findTreeById, we can probably change it from db.one() to db.oneOrNone() since it's possible for the tree to only be in the vector tiles. Right now it would throw a 500 error is no tree was found because we coded it so that we expect be one row returned, but it should also be fine if no rows were returned. Then a 500 would be thrown if the sql call was formatted wrong or more than 1 tree with the same id was found. Would do you think?

[zoobot]

I think when sql returned bad data or something, it would break and not return anything or send back a response other than a 500.

for findTreeById, we can probably change it from db.one() to db.oneOrNone() since it's possible for the tree to only be in the vector tiles.

sounds good if we can handle collision errors somehow with oneOrNone?

Then a 500 would be thrown if the sql call was formatted wrong

Sounds good.

or more than 1 tree with the same id was found. Would do you think?

If we have an id collision, we should probably handle it, return a specific error about the collision, so it gets logged and we can investigate that id. There is some probability of collisions so that's not really an unexpected error (500).

[jazhen]

If we have an id collision, we should probably handle it, return a specific error about the collision, so it gets logged and we can investigate that id. There is some probability of collisions so that's not really an unexpected error (500).

I can change it so we return all rows and then throw a 400? error + message about collision if more than one row is returned. Otherwise, if 0 or 1 rows are returned, then we do what were doing now.

[jazhen]

I added a check for collisions
https://github.com/waterthetrees/wtt_server/pull/101/files#diff-46fb011f2bd00532779ee42889b32096d8caac5b9c6404d89512c072fa30ec30R33-R40

and a test
https://github.com/waterthetrees/wtt_server/pull/101/files#diff-2df4b304dc84eb5f1bad37299d5d1a8a8c83f8e77423da739c97f0e464561babR103-R132

Let me know if this was what you were thinking.

[zoobot]

Thanks, I'll check the logs over the next few months to see how many collisions there are.

At the next meeting lets talk about whether we should return the most recent tree if there's a collision.
We'd removed the ability to edit tree names on the front end because we didn't know how to deal with having more than one, should someone change the name and then the old name gets synced back in from the city's data source. Maybe this collision handling will give us an avenue to have more than one tree with the same id. Will have to find the other issues related to that. Probably before we go that route, we should have a unique tree table.

[jazhen]

Ok, we can discuss the collisions in more details next meeting. I fixed the tests because I'm looking to aggressively? refactor so that our business logic is separated from external sources. This will hopefully allow for more unit tests and increased confidence in making any changes further down the line. I've been saying I wanted to do this for a while now, and so I think I just need to make quick, iterative changes more often instead of waiting to find the "perfect" solution. So it's possible that I break some things during this process 🙃, but I think it'll payoff when we're making changes in the future.

[zoobot]

Ok, we can discuss the collisions in more details next meeting. I fixed the tests because I'm looking to aggressively? refactor so that our business logic is separated from external sources. This will hopefully allow for more unit tests and increased confidence in making any changes further down the line. I've been saying I wanted to do this for a while now, and so I think I just need to make quick, iterative changes more often instead of waiting to find the "perfect" solution. So it's possible that I break some things during this process 🙃, but I think it'll payoff when we're making changes in the future.

Awesome! good refactor attitude!