Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add owner field to packages table… #215

Merged
merged 15 commits into from
Jan 8, 2024

Conversation

savetheclocktower
Copy link

@savetheclocktower savetheclocktower commented Dec 30, 2023

…along with a script for extracting the owner from the repo URL.

Description of the Change

Add an owner field to the packages table. The goal here is eventually to allow the frontend to query all packages that are owned by a specific GitHub organization.

This is a distinct concept from the backend's (or frontend's) idea of a “user,” since a user can publish a package to an organization they don't own. For example, the linter-eslint-node package was owned by the AtomLinter organization, and a user would expect to see as much when looking at the package card on the frontend. If they clicked on AtomLinter, they'd want to see other packages owned by AtomLinter. The fact that I, user savetheclocktower, was usually the one publishing those packages is irrelevant.

@confused-Techie
Copy link
Member

Taking a quick look here, overall these changes look fantastic!

Love especially that you took the chance to fix some minor errors in docs, as well as renaming the variables for pagination like you mentioned in #213.

The only thing I'd be super concerned about besides testing which we are discussing on Discord, is to ensure that all other pagination returns match the new names used here. Since if memory serves theres a couple more endpoints that utilize this.

src/database.js Fixed Show fixed Hide fixed
@savetheclocktower savetheclocktower marked this pull request as ready for review December 31, 2023 03:23
@savetheclocktower
Copy link
Author

OK, the tests are passing, so I'm going to take this out of draft.

@confused-Techie, let me know what you think of the add-owner-field.js script and whether it should behave any differently. I don't have a great way of testing it, but it was working in the contrived scenario I set up.

Copy link
Member

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

After fully reviewing the code this looks great!
I may slightly change the script to ensure it's easily runnable from the CLI the same way others are, but otherwise this is fantastic!

I appreciate the time you took to ensure things are as well tested as allowed to be currently, additionally taking the time to fix any linter warnings, and ensuring ESLint was as happy as can be.

This PR is remarkably complete, handling edge cases, handling initial migration, and even ensuring things are displayed later on. Especially love the time you took to ensure the data is easily available on every single different query to the DB that returns package data.

Seriously impressed, and with the tests being happy, it seems to me we are good to merge.


An aside for expectations, after merging, I'll need to run the migrations for the DB to include these changes. And hoping there are no errors, afterwards I can go ahead and push these changes to production for everyone to take advantage of, and especially then get an owner endpoint functional on the backend.

Thanks again for all your work here!

@savetheclocktower
Copy link
Author

No problem! Don't have write access to this repo, so this one is on you to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants