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

[#IP-275] Managed service preferences before message creation #142

Merged
merged 24 commits into from
Jul 9, 2021

Conversation

michaeldisaro
Copy link
Contributor

List of Changes

  • Added checks for user's services preferences during the message creation activity

Motivation and Context

We want to create messages only for users that allow to be reached, and return a coherent blockedInboxOrChannels based on preferences if they are present.

How Has This Been Tested?

It has been tested by performing:

  • yarn install --frozen-lockfile
  • yarn build
  • yarn lint
  • yarn test

Types of changes

  • Chore (nothing changes by a user perspective)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@pagopa-github-bot
Copy link
Contributor

Warnings
⚠️

Please include a Pivotal story at the beginning of the PR title (see below).

Example of PR titles that include pivotal stories:

  • single story: [#123456] my PR title
  • multiple stories: [#123456,#123457,#123458] my PR title

Generated by 🚫 dangerJS

Copy link
Contributor

@gquadrati gquadrati left a comment

Choose a reason for hiding this comment

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

LGTM

StoreMessageContentActivity/handler.ts Show resolved Hide resolved
StoreMessageContentActivity/handler.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/handler.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/handler.ts Show resolved Hide resolved
Co-authored-by: gquadrati <[email protected]>
StoreMessageContentActivity/handler.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved
StoreMessageContentActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved
@balanza
Copy link
Contributor

balanza commented Jul 8, 2021

The code seems to work as expected, anyway by a reviewer perspective is hard for me to focus on differences because the whole activity has been refactored. I'm afraid I cannot spot eventual bug/regression/edge-case we may have included in those changes.

To help on this I rewrote the test suite so that is easier to figure out which scenario we're testing and which we're not. So far I just replicated tests we used to have, and group such in three families: successes, failures and exceptions.

Changes are in 7e98711

@michaeldisaro
Copy link
Contributor Author

The code seems to work as expected, anyway by a reviewer perspective is hard for me to focus on differences because the whole activity has been refactored. I'm afraid I cannot spot eventual bug/regression/edge-case we may have included in those changes.

To help on this I rewrote the test suite so that is easier to figure out which scenario we're testing and which we're not. So far I just replicated tests we used to have, and group such in three families: successes, failures and exceptions.

Changes are in 7e98711

Refactoring happened mainly to avoid linting errors due to too much parameters in function signature. Other refactoring happened to extract the old code and make the logic more understandable.

Your test refactoring seems legit.

Copy link
Contributor

@BurnedMarshal BurnedMarshal left a comment

Choose a reason for hiding this comment

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

LGTM

@BurnedMarshal BurnedMarshal merged commit ae1020f into master Jul 9, 2021
@BurnedMarshal BurnedMarshal deleted the ip-275-b branch July 9, 2021 07:08
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.

6 participants