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

initial commit of membership endpoint #20

Closed
wants to merge 1 commit into from

Conversation

ricklarsen
Copy link

@ricklarsen ricklarsen commented Apr 14, 2024

What/Why/How?

Initial pass at creating a membership system with multiple tiers for museum patrons.

issue #17

Submitted for feedback!

Reference

Testing

Screenshots (optional)

Check yourself

  • Code is linted
  • Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Copy link
Contributor

@lornajane lornajane 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 adding the memberships idea to the API description. I had some feedback on the repeated schema descriptions - these can be combined to help keep things consistent. The data is very similar between the membership types so there's no need to describe separate payloads.

type: float
example: 100
PatronMembership:
ription: Benefits of a patron membership
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ription: Benefits of a patron membership
description: Benefits of a patron membership

- Patron
- Benefactor
example: Family
FamilyMembership:
Copy link
Contributor

Choose a reason for hiding this comment

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

These three schemas named *Membership are identical in structure. To ensure consistency, they should be one schema.

description: Annual cost of a membership
type: float
example: 1000
BuyFamilyMembershipRequest:
Copy link
Contributor

Choose a reason for hiding this comment

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

These identically-structured schemas Buy*MembershipRequest should be a single schema since the requests will all the be same structure of payload, with different details included. Please combine them.

- membershipLevel
- startDate
- email
BuyFamilyMembershipResponse:
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine the Buy*MembershipResponses into a single schema unless some membership types have distinctly different structures. The goal of re-using schemas in an API is to ensure consistency - with these duplicated schemas, it would be easy to change one, or miss changing one, and introduce inconsistency. Since the structure is the same, they should use the same schema.

@@ -628,6 +859,24 @@ components:
- date: "2023-09-22"
timeOpen: "10:00"
timeClose: "16:00"
BuyFamilyMembershipExample:
Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple examples are great to illustrate the different expected data fields when the structures are the same. Thanks for adding these (no changes needed, I'm just happy to see the examples)!

@lornajane
Copy link
Contributor

Closing due to inactivity

@lornajane lornajane closed this Jun 7, 2024
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.

None yet

3 participants