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

create AddressDirectAnswer interface #203

Merged
merged 29 commits into from
Aug 16, 2022
Merged

Conversation

juliannzhou
Copy link
Contributor

@juliannzhou juliannzhou commented Aug 12, 2022

This PR creates AddressDirectAnswer interface for direct answers with address values and an address type to define generic type variable.

J=SLAP-2310
TEST=manual

Test: created a test interface with AddressDirectAnswer and works as expected for when inputting address type values. The Address type interface is generated by CLI types command for Address field of Person entity. All address fields are verified by checking api response in Search except for sublocality, region, and line 3, as I cannot find a search result that returns an address direct answer with these three fields.

@juliannzhou juliannzhou requested a review from a team as a code owner August 12, 2022 20:35
@coveralls
Copy link

coveralls commented Aug 12, 2022

Pull Request Test Coverage Report for Build 2870583232

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.408%

Totals Coverage Status
Change from base Build 2869609543: 0.0%
Covered Lines: 214
Relevant Lines: 231

💛 - Coveralls

}

/**
* A type interface with address fields to use in {@link FieldValueDirectAnswer.value}.
Copy link
Contributor

@oshi97 oshi97 Aug 15, 2022

Choose a reason for hiding this comment

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

I think we can omit telling people this is a type interface, it's obvious just looking at it!

import { BuiltInFieldType } from './BuiltInFieldType';

/**
* An interface for direct answers for a {@link FieldValueDirectAnswer} with address values.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a little wordy. I think it can be simplified by removing the first part ("An interface for direct answers for")

*
* @public
*/
export interface Address {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity are all of these properties truly optional?

Copy link
Contributor

Choose a reason for hiding this comment

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

what do the yext types generate answers types say? these are technically different types but for Address I would assume they match up.

separate note: At least one of these properties must be not-undefined, otherwise we would never get a direct answer, but I don't think there's a good way to represent that through typescript

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it out on slanswers-test experience and I think line 1, city, region, and postalCode are required, although they are generated through CLI as optionals.

Copy link
Contributor

Choose a reason for hiding this comment

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

What makes you think they're required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you add address to a person or a location, these fields are required.

import { DirectAnswer } from './directanswer/DirectAnswer';

/**
* A direct answer {@link DirectAnswer} interface with address value.
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be reworded? right now, the text would read "A direct answer DirectAnswer interface with ..."

@oshi97
Copy link
Contributor

oshi97 commented Aug 15, 2022

@juliannzhou what queries did you try? I wanted to try poking around as well

@juliannzhou
Copy link
Contributor Author

@juliannzhou what queries did you try? I wanted to try poking around as well

I did 'yen address', 'bob address', 'event address', 'yext address'

@oshi97
Copy link
Contributor

oshi97 commented Aug 15, 2022

We need to add your new DirectAnswer interface to the FieldValueDirectAnswer union type. You can look at my pr as an example #208
If you want you can wait for my pr to be merged in, and then use the test-site infra I wrote

@oshi97
Copy link
Contributor

oshi97 commented Aug 16, 2022

Could you add to the test-site's initDirectAnswer switch case?

sublocality?: string,
city: string,
region: string,
postalCode: string,
Copy link
Contributor

@yen-tt yen-tt Aug 16, 2022

Choose a reason for hiding this comment

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

I'm not certain if all non US areas would have postal code? I added a location for Bolivia to the account to test. Feel free to add a Bolivia address to something else and try it out

Copy link
Contributor

@yen-tt yen-tt Aug 16, 2022

Choose a reason for hiding this comment

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

Screen Shot 2022-08-16 at 12 58 09 PM

Alright, looks like region and postalCode is not certain. let's make them optional. Lets add your test query to the test file as well!

Copy link
Contributor

@oshi97 oshi97 Aug 16, 2022

Choose a reason for hiding this comment

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

If we're not sure about things and composeAbstractSchema says they're all optional I don't mind just making them all optional

Copy link
Contributor

Choose a reason for hiding this comment

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

I would assume at least line1 should always be defined for an address? can't say the same for the other ones though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Julianne talked to KM channel. Looks like countryCode should always be defined in practice and everything else is optional. So we should update to reflect that

*
* @public
*/
export interface Address {
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to export this in the fieldvaluedirectanswer/index.ts file was well since this is part of the interface above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was added in the file. Can you check again if the file changes are refreshed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see Address in the index file. Also this warning indicates it's not exported.
Screen Shot 2022-08-16 at 3 26 32 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you didn't push your new changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see!

@juliannzhou juliannzhou merged commit f159737 into develop Aug 16, 2022
@juliannzhou juliannzhou deleted the dev/address-direct-answer branch August 16, 2022 19:56
@yen-tt yen-tt mentioned this pull request Sep 6, 2022
yen-tt added a commit that referenced this pull request Sep 8, 2022
## Version 2.0.0

### Changes
- `additionalQueryParameter` is now a public field in `SearchConfig` models (#217)

### Breaking Changes
- Updated default and sandbox URL endpoints from `../answers/..` to `../search/..` as part of rebranding process (#196 )
- Restructured StaticFilters models for better developer experience and enforce proper restriction in the kind of combinations of filters supported by the backend
  - Now, static filters can be either a field value filter or a nested object that is composed by combining field value filters. (#201 )
  - `ConjunctionStaticFilter` and `DisjunctionStaticFilter` models were created to reflect such limitation. For example, ANDs of ORs combination is allowed, ORs of ANDs combination is not allowed.  (#204 )
- Updated `FieldValueDirectAnswer` model to properly handle different `value` types. Previously, FieldValueDirectAnswer interface enforces that the `value` field will always be of type `string`. Now, `FieldValueDirectAnswer` is a union type of predefined interfaces with known `value` type and `UnknownFieldValueDirectAnswer` with a generic `value` type for other `fieldType` outside of `BuiltInFieldType`. (#200 #202 )
   - The newly added built-in interfaces can be found in the document page [**here**](https://github.com/yext/search-core/blob/develop/docs/search-core.fieldvaluedirectanswer.md)  (#206 #208 #210 #211 #205 #209 #203 #220 )
- Narrow down `FeaturedSnippetDirectAnswer` TypeScript model to be a union type of   `MultiLineTextSnippetDirectAnswer` and `RichTextSnippetDirectAnswer` as a featured snippet direct answer can only be of fieldType `multi_line_text` or `rich_text`. (#207 #212 )
- All exports marked as `@deprecated` in previous version(s) as part of the rebranding process is now removed in V2 (#216 )
  - For more details, the deprecated identifiers are listed in [v1.8 release notes](https://github.com/yext/search-core/releases/tag/v1.8.0)
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

5 participants