Skip to content
This repository has been archived by the owner on Jul 26, 2024. It is now read-only.

test: replace contract test sequence diagram with Miro version #426

Merged
merged 4 commits into from
Jul 27, 2022

Conversation

Trinaa
Copy link
Collaborator

@Trinaa Trinaa commented Jul 25, 2022

Description

Update sequence diagram in contract-tests README to Miro version.

Issue(s)

CONSVC-1896

@Trinaa Trinaa requested a review from hackebrot July 25, 2022 21:06
@Trinaa Trinaa requested a review from a team as a code owner July 25, 2022 21:06
@Trinaa Trinaa force-pushed the contract-test-sequence-diagram branch from 5df9ef3 to 42bb64d Compare July 25, 2022 21:19
pjenvey
pjenvey previously approved these changes Jul 25, 2022
Copy link
Member

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

Hi @Trinaa! 👋🏻

We have added new fields to the cache audience key since I created the original. Do you mind updating that information?

See

pub struct AudienceKey {
/// Country in ISO 3166-1 alpha-2 format
pub country_code: String,
/// Region/subdivision (e.g. a US state) in ISO 3166-2 format
pub region_code: Option<String>,
/// The DMA code (u16)
pub dma_code: Option<u16>,
/// The form-factor (e.g. desktop, phone) of the device
pub form_factor: FormFactor,
/// Platform OS
pub os_family: OsFamily,
/// Only serve legacy
pub legacy_only: bool,
}

pjenvey
pjenvey previously approved these changes Jul 26, 2022
@Trinaa
Copy link
Collaborator Author

Trinaa commented Jul 26, 2022

Hi @hackebrot,

  • added the new fields to cache audience key
  • homogenized the arrow styles (a mix might have contributed to the crookedness)
  • adjusted arrow labels and reduced white space
  • bolded the 'scenarios.yml' and 'responses.yml' in file comments
  • add partner DELETE records to the sequence diagram

@Trinaa Trinaa requested a review from hackebrot July 26, 2022 16:47
Copy link
Member

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

Hi @Trinaa! 👋🏻

This is looking much better! Thank you.

Two minor corrections:

  • The note on the left reads verify the responses from contile. Now that the diagram also shows an request from client to partner, should this be verify the responses from contile or partner?
  • The description for a 204 HTTP response status is No Content. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/204

Copy link
Member

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

LGTM! ✏️

@Trinaa Trinaa merged commit 8ab95c1 into main Jul 27, 2022
@Trinaa Trinaa deleted the contract-test-sequence-diagram branch July 27, 2022 15:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants