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

feat: add categories and categories:create #530

Merged
merged 37 commits into from
Jul 21, 2022

Conversation

garrett-wade
Copy link
Contributor

@garrett-wade garrett-wade commented Jun 29, 2022

🚥 Fix #13

🧰 Changes

Added support for two new commands categories and categories:create.

  • categories returns a raw json of all categories for a version including pagination.
  • categories:create allows you to create a new category with a specified title and type for your project version. The title will be converted to a slug in param case. If a category already exists with the same slug and type for
    the specified version then a new category is not created and the id of the existing category is returned. This includes if the slug contains incremented suffix such as -1 or -2. If there are multiple categories with a matching slug and type then only one matching category is returned.

🧬 QA & Testing

  • categories
    1. Login to a ReadMe project with rdme login
    2. Run rdme categories
  • categories:create
    1. Login to readme project with rdme login
    2. Run rdme categories:create "test title" --categoryType guide and see a new category was created with a slug of test-title
    3. Run rdme categories:create "test title" --categoryType guide and see a new category was not created but rather it returns the slug and id of the category created in the previous step.
    4. Run rdme categories:create "test title" --categoryType reference and see a new category was created with a slug of test-title-1
    5. Run rdme categories:create "test title" --categoryType reference and see a new category was not created but rather it returns the slug and id of the category created in the previous step.
    6. Run rdme categories:create "test title" and see a categoryType of "guide" or "reference" is required

@erunion erunion added the enhancement New feature or request label Jun 30, 2022
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
src/cmds/categories/index.js Outdated Show resolved Hide resolved
src/lib/commands.js Show resolved Hide resolved
src/cmds/categories/index.js Outdated Show resolved Hide resolved
@garrett-wade
Copy link
Contributor Author

@erunion updates complete

@erunion erunion requested a review from kanadgupta July 5, 2022 17:21
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @garrett-wade! I have a few overarching questions about approach on the create command below. Would love your feedback as a user of this tool!

src/cmds/categories/create.js Outdated Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

Thanks for the context @garrett-wade! I think the example makes sense and I can see title-uniqueness checks being useful. Could you make the following changes:

  • Could you refactor your partial-slug matching to do exact title-matching instead? Partial slug matching may incur some weird false positives and title-matching seems more robust.

  • Could you wrap the aforementioned matching logic in a flag called preventDuplicateTitles? In general I think the default behavior should follow the behavior of our API and the dashboard, but this way users can opt-in to ensuring that titles are unique.

  • Can you merge the main branch into this branch and re-run npm install with at least npm@7 and update the lockfile? It appears tests are still failing.

@kanadgupta kanadgupta self-requested a review July 7, 2022 19:46
- Wrap matching logic in `preventDuplicates` flag
- Update test coverage to reflect changes
- Update README to reflect changes
@garrett-wade
Copy link
Contributor Author

@kanadgupta Agree with the title matching change, thanks for the feedback!

Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

Thanks for making those suggested changes @garrett-wade! Some more comments below:

src/cmds/categories/create.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
}

async function createCategory() {
if (preventDuplicates) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this boolean check up so it wraps all of this logic? In other words, we shouldn't need to fetch the category list at all if preventDuplicates is set to false.

Copy link
Member

Choose a reason for hiding this comment

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

Are you still working on this? This comment got marked as resolved but it's still applicable so I'm going to unresolve it! Just to reiterate, the getNumberOfPages and matchCategory functions should not be running at all if preventDuplicates is set to false.

src/cmds/categories/create.js Outdated Show resolved Hide resolved
src/cmds/categories/index.js Outdated Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
src/cmds/categories/create.js Outdated Show resolved Hide resolved
}

async function createCategory() {
if (preventDuplicates) {
Copy link
Member

Choose a reason for hiding this comment

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

Are you still working on this? This comment got marked as resolved but it's still applicable so I'm going to unresolve it! Just to reiterate, the getNumberOfPages and matchCategory functions should not be running at all if preventDuplicates is set to false.

@kanadgupta kanadgupta self-requested a review July 12, 2022 14:07
@garrett-wade
Copy link
Contributor Author

garrett-wade commented Jul 18, 2022

@kanadgupta updated based on requests above, for preventDuplicates I do not think either of the above methods are ran if the flag is checked with the current implementation.

The initial version of this was making a duplicate call for the first page of categories. This refactors it so it uses the initial call to both grab the count and the first page of results.

I also extracted this logic and put it in a shared lib function.
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @garrett-wade! I added a few commits where I reverted the package* file changes, consolidated and refactored some logic, and made some small copy edits.

@kanadgupta kanadgupta requested a review from erunion July 21, 2022 01:05
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

thanks for all the work on this

@garrett-wade
Copy link
Contributor Author

@erunion @kanadgupta Thanks for all the collaboration and feedback on this. Learned a lot and enjoy working with both of you!

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

Successfully merging this pull request may close these issues.

Add an underlying GET /categories endpoint and expose it to rdme
3 participants