Skip to content
This repository has been archived by the owner on Jan 31, 2023. It is now read-only.

improved insertNewPackageName #76

Merged
merged 5 commits into from
Nov 22, 2022

Conversation

Digitalone1
Copy link
Contributor

Requirements

  • Have you ran tests against this code?
  • This PR contains zero code changes.

I got errors about express-rate-limit module not found.

Description of the Change

I gave a look at insertNewPackageName. I thought it may break uniqueness of packages.name inserting duplicate names into names. Only later I found out names.name is the primary key, so the new name should not be used to be inserted.

Anyway I added the code to update the name in the packages table (I suppose we want there the current name used, so the new name). And I wrapped the whole process into a transaction, which is useless if I made first the insertion in names and then exit on fail, but I wrongly supposed the names could be duplicated, so I made first the update into packages.

But the transaction is good anyway and also robust in case of unexpected errors, so I think it's acceptable as it is. Please give it a review.

Then I switched some variables to camel case notation.

Copy link
Owner

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

This overall looks fantastic!

Had me slightly concerned seeing the throw statements, but then realized you have everything wrapped with a catch, so smart move there, to help Failing Server Status Object Returns.

The tests look fine for what has been changed, but very hard to tell if this will change the functionality here as it's untested. But it looks great, and I'll just double check the SQL Syntax before commiting.

Thanks for the contribution!

@Digitalone1
Copy link
Contributor Author

The throw will trigger the catch statement, which will trigger the rollback. ;)

Same done with other transactions.

@confused-Techie confused-Techie merged commit ee2197f into confused-Techie:main Nov 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants