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

"Type" missing in rates/getAll documentation.md #470

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

BrittaBecker
Copy link
Contributor

For rates/getAll, the response includes the type of rate (e.g. private, public, availability block). This is not mentioned in the documentation but in swagger. This value is very helpful to determine which rates are availability block rates and which ones are not.

For rates/getAll, the response includes the type of rate (e.g. private, public, availability block). 
This is not mentioned in the documentation but in swagger. This value is very helpful to determine which rates are availability block rates and which ones are not.
Copy link
Contributor

@MikeAdamsMews MikeAdamsMews left a comment

Choose a reason for hiding this comment

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

Hi @BrittaBecker, I have edited your patch directly, I thought that would be the easiest. Since this way of working is still new, I thought it would be helpful to describe what I changed in detail here:

  1. Added a changelog entry - I think this change is substantial enough that it should have an entry in the changelog.
  2. Edited Type so that it refers to a separate string enumeration entitled Rate type, and added that string enumeration; this is the normal way this would be handled in the docs.
  3. Following the suggestion of @vitezp, I added the other missing property Description.

There is one thing still missing, however - we need to add examples of the two new fields to the JSON response example. Is this something you could do?

And finally, I would like someone from Dev to approve the changes before they are accepted into the main branch.

Thanks :-)

I've added the "type" parameters in the code example. 

Additionally, I noticed that the "options" parameter was also not described, so I added this as well. 
@MikeAdamsMews could you please check and adjust the "options" description if needed? Thanks!
"ExternalNames": {},
"Description": {},
"ExternalIdentifier": null,
"Options": {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is Options? This has sneaked in to the PR.

"Name": "Fully Flexible Rate",
"ShortName": "FLEXFLEX",
"ExternalNames": {},
"Description": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to use real examples, rather than just blank. I guess it would be something like this:

   "Description": {
      "en-US": "Fully flexible rate",
      "de-DE": "Völlig flexibler Tarif"
   },

| `ExternalIdentifier` | string | optional, max 255 characters | Identifier of the rate from external system. |
| `Options` |[Localized text](_objects.md#localized-text)| required | Rate options. |
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, what is Options? I checked the YAML and it's not in there.

@MikeAdamsMews
Copy link
Contributor

Hi @BrittaBecker, when I reviewed this PR in August I raised a couple of questions, could you take a look please.

@BrittaBecker
Copy link
Contributor Author

Hi @MikeAdamsMews, I've checked the points.

  1. Example for description (feel free to change the text):
    "Description": {
    "de-DE": "Rate enthält Halbpension.",
    "en-US": "Rate including half board."
    },
  2. I don't see "Options" in swagger or the UX either, but I do get it returned in rates/getAll as part of the rate data, here is an example:
    "Options": {
    "HidePriceFromGuest": false,
    "IsBonusPointsEligible": false
    }
    @vitezp can you clarify where this is coming from and if it should be part of the documentation?

@MikeAdamsMews MikeAdamsMews marked this pull request as draft May 23, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants