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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - feat: added importable types for survey context (PL-37) #359

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - feat: added importable types for survey context (PL-37) #359

wants to merge 8 commits into from

Conversation

zhihil
Copy link
Contributor

@zhihil zhihil commented Aug 24, 2022 •
edited
Loading

Fixes or implements PL-37

Brief description. What is this change?

Adds typing for surveyorContext on the Version data.

Implementation details. How do you make this change?

Added a SurveyContext type for the version.prototype.surveyorContext property.

The SurveyContext can be extended by the SurveyContextExtension type.

Each channel extends the survey context with its own properties: AlexaSurveyContextExtension or GaDfesSurveyContextExtension.

The original source of truth of SurveyContext is actually in this file: general-service/lib/clients/platform/types.ts. I've just broken it up so that the domain-specific properties belong to the domain-specific type package, and allowed the survey context to be shared.

Setup information

N/A

Deployment Notes

  1. libs - Can be merged now
  2. general-service - Can be merged after libs
  3. alexa-service + google-service - Merged after the prototype-programs migration is done

Related PRs

Checklist

  • this is a breaking change and should publish a new major version
  • appropriate tests have been written

@zhihil zhihil added the in progress In progress, should not be merged label Aug 24, 2022
@zhihil zhihil self-assigned this Aug 24, 2022
zhihil
zhihil commented Aug 24, 2022
View reviewed changes
packages/base-types/src/models/version/prototype.ts
type: string;
data: PrototypeData<Locale>;
model: PrototypeModel;
context: PrototypeContext<Command>;
platform: string;
settings: PrototypeSettings;
surveyorContext: SurveyContext<SurveyContextExtension>;
Copy link
Contributor Author

@zhihil zhihil Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This property is needed inside general-service, which generates the surveyorContext while it generates the prototype.

I think it makes the most sense to pull the surveyorContext out of a finished prototype, rather than returning the surveyor context from general-service before it gets passed into the prototype rendering code.

It leads to less brittle code, because if the prototype rendering modifies surveyorContext for whatever reason, we capture that.

@zhihil zhihil added blocked dont merge ready for review and removed in progress In progress, should not be merged labels Aug 29, 2022
zhihil
zhihil commented Aug 30, 2022
View reviewed changes
packages/alexa-types/src/version/prototype.ts
export interface AlexaSurveyContextExtension {
products: Record<string, Product>;
permissions: PermissionType[];
interfaces: InterfaceType[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some channels populate the surveyor context with additional data. The AlexaSurveyContextExtension encodes these additional properties for Alexa. There is an extension that covers additional properties from Google and DFES.

zhihil
zhihil commented Aug 30, 2022
View reviewed changes
packages/alexa-types/src/version/index.ts
@@ -24,7 +26,8 @@ export interface PlatformData extends VoiceVersion.PlatformData<Voice> {
publishing: Publishing;
}

export interface Version extends VoiceVersion.Version<Voice, BaseModels.Version.Prototype<AnyCommand, VoiceflowConstants.Locale>> {
export interface Version
extends VoiceVersion.Version<Voice, BaseModels.Version.Prototype<AnyCommand, VoiceflowConstants.Locale, AlexaSurveyContextExtension>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I'm just passing in the AlexaSurveyContextExtension to extend the SurveyContext with Alexa-specific properties for this default "AlexaVersion" exported by alexa-types.

In the code below, you'll see me do this a lot of places to avoid type issues in the *-services.

zhihil
zhihil commented Aug 30, 2022
View reviewed changes
packages/base-types/src/models/version/index.ts
@@ -57,7 +62,7 @@ export interface Model<_PlatformData extends PlatformData, Command extends BaseC
domains?: Domain[];
folders?: Record<string, Folder>;
variables: Variable[];
prototype?: Prototype<Command, Locale>;
prototype?: Prototype<Command, Locale, SurveyContextExt>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some boilerplate here to allow us to specify the SurveyContextExt in SurveyContext from an enclosing Version.Model type.

pmvrmc
pmvrmc approved these changes Sep 12, 2022
View reviewed changes
Copy link

@pmvrmc pmvrmc left a comment

Choose a reason for hiding this comment

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

looks good, lets merge it :)

@pmvrmc pmvrmc marked this pull request as ready for review September 13, 2022 14:10
@pmvrmc pmvrmc force-pushed the brennan/use-surveyor-context/PL-37 branch from 5e10ce7 to 28a86c3 Compare September 13, 2022 14:11
@pmvrmc
Copy link

pmvrmc commented Sep 13, 2022

bors r+

bors-vf bot pushed a commit that referenced this pull request Sep 13, 2022
@zhihil
feat: added importable types for survey context (PL-37) (#359)
0484f34 
<!-- You can erase any parts of this template not applicable to your Pull Request. -->

**Fixes or implements PL-37**

### Brief description. What is this change?

<!-- Build up some context for your teammates on the changes made here and potential tradeoffs made and/or highlight any topics for discussion -->

Adds typing for `surveyorContext` on the `Version` data.
@zhihil zhihil force-pushed the brennan/use-surveyor-context/PL-37 branch from 28a86c3 to 27f0a3e Compare September 13, 2022 14:14
@bors-vf
Copy link

bors-vf bot commented Sep 13, 2022

Canceled.

@pmvrmc
Copy link

pmvrmc commented Sep 13, 2022

bors r+

bors-vf bot pushed a commit that referenced this pull request Sep 13, 2022
@zhihil
feat: added importable types for survey context (PL-37) (#359)
646bd4c 
<!-- You can erase any parts of this template not applicable to your Pull Request. -->

**Fixes or implements PL-37**

### Brief description. What is this change?

<!-- Build up some context for your teammates on the changes made here and potential tradeoffs made and/or highlight any topics for discussion -->

Adds typing for `surveyorContext` on the `Version` data.
@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bors-vf
Copy link

bors-vf bot commented Sep 13, 2022

Pull request successfully merged into master.

Build succeeded:

@bors-vf bors-vf bot changed the title feat: added importable types for survey context (PL-37) [Merged by Bors] - feat: added importable types for survey context (PL-37) Sep 13, 2022
@bors-vf bors-vf bot closed this Sep 13, 2022
@bors-vf bors-vf bot deleted the brennan/use-surveyor-context/PL-37 branch September 13, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmvrmc pmvrmc pmvrmc approved these changes

Assignees

@zhihil zhihil

Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zhihil @pmvrmc