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

CMB-586: add booklets CRUD documentation #498

Merged
merged 6 commits into from
Jul 3, 2024
Merged

CMB-586: add booklets CRUD documentation #498

merged 6 commits into from
Jul 3, 2024

Conversation

sachinmurali
Copy link
Contributor

@sachinmurali sachinmurali commented Jun 24, 2024

Fixes CMB-586

Checklist

  • Up to date with main
  • All the tests are passing
    • Delete all resources created in tests
  • Prettier
  • Spectral Lint
  • npm run bundle outputs nothing suspect
  • npm run postman outputs nothing suspect

Changes

  • Adds booklets CRUD documentation
  • The code samples are not added because the booklets resource does not exist in the SDKs. Once the SDK is updated with the booklet resource, the code samples will need to be introduced.

Areas of Concern (optional)

As mentioned above, the code samples are missing. This is because they are not part of the SDKs. Once they are introduced in the SDKs, the docs will need to be updated.

I encourage the reviewers to pull down the branch and preview the HTML to catch any errors or mistakes prior to approval.

@sachinmurali sachinmurali requested a review from a team as a code owner June 24, 2024 17:35
@sachinmurali sachinmurali requested a review from a team June 24, 2024 17:35
@sachinmurali
Copy link
Contributor Author

The booklets launch is scheduled for end of July. This PR is expected to be merged when we're good to go with the launch of booklets. However, at this moment in time, it is good for a review.

Copy link
Contributor

@mrjamesjcho mrjamesjcho left a comment

Choose a reason for hiding this comment

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

A couple of notes but looking good so far!

@@ -0,0 +1,4 @@
type: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

color shouldn't be part of the booklet payload so we can remove this attribute and remove color from the samples. I think we still have it in the booklet serializer but currently it will always be undefined. I'll create a ticket to remove color and return_address.

- $ref: "../../../shared/attributes/model_ids/vrsn_id.yml"

campaign_id:
$ref: "../../../shared/attributes/campaign_id.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be updated to note that campaign ids for booklets will also be prefixed with camp_.
It also looks like the description in /shared/parameters/campaign_id.yml should also be updated for the list endpoint parameter.

Copy link
Contributor

@mrjamesjcho mrjamesjcho left a comment

Choose a reason for hiding this comment

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

Just need to remove the color property in the response but otherwise looks good 😄. I'll approve to not block.

Comment on lines 64 to 65
color: true
mail_type: usps_first_class
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't catch color in the response last time time but this should be removed as well.

Suggested change
color: true
mail_type: usps_first_class
mail_type: usps_first_class

date_created: "2018-12-08T03:01:07.651Z"
date_modified: "2018-12-08T03:01:07.651Z"
object: address
color: true
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

date_modified: "2017-09-05T15:54:53.264Z"
deleted: true
object: address
color: true
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

-d 'qr_code[pages]=3,4-5' \
-d 'fsc=true'
label: CURL
# - lang: Typescript

Choose a reason for hiding this comment

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

not blocking, just curious why this is commented out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Booklets are not part of the SDKs. The SDKs are in a strange state where they have not been maintained over the past several months. I've surfaced this to my manager @nperri6 (and @gtstewart16) and he's been in talks with the ELT to determine the next steps for the SDKs. In the meantime, we do not want to convey the wrong information to the users of the API, given that the examples are pointing to the usage of these objects under the respective SDKs. That's why the Blueprint team agreed to leave out the code examples.

Choose a reason for hiding this comment

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

got it, makes sense!

@sachinmurali sachinmurali merged commit c02f78f into main Jul 3, 2024
6 checks passed
@sachinmurali sachinmurali deleted the CMB-586-1 branch July 3, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants