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

[#175497433] Add Profile unsubscription on feed #86

Merged
merged 4 commits into from
Nov 3, 2020

Conversation

AleDore
Copy link
Contributor

@AleDore AleDore commented Nov 2, 2020

This PR adds retrieve for records with partitionKeyP-{date}-U on SubscriptionFeedByDay while serving informations about subscription feed. These kind of records are written, when users request to delete its own data and in that case, the event must be notified to subscription feed.

@pagopa-github-bot
Copy link
Contributor

pagopa-github-bot commented Nov 2, 2020

Affected stories

  • 🌟 #175497433: Come SAS, voglio ricevere l'aggiornamento sul SubscriptionFeed qualora un utente abbia effettuato la cancellazione del proprio profilo

Generated by 🚫 dangerJS

Copy link
Contributor

@gunzip gunzip left a comment

Choose a reason for hiding this comment

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

may we add some tests?

// add new users to the new subscriptions, skipping those that
// unsubscribed from this service
// unsubscribed from this service or deleted its account
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// unsubscribed from this service or deleted its account
// unsubscribed from this service or deleted their account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8857930

// add all users that unsubscribed from this service, skipping those
// that created the profile on the same day as the service will not
// yet know they exist
// yet know they exist or deleted its account
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// yet know they exist or deleted its account
// yet know they exist or deleted their account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8857930

if (!serviceUnsubscriptionsSet.has(ps)) {
if (
!serviceUnsubscriptionsSet.has(ps) &&
!profileUnsubscriptionsSet.has(ps)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this check is useless since you either have a profile subscription OR a profile unsubscription but never both in the same day

Copy link
Contributor Author

@AleDore AleDore Nov 2, 2020

Choose a reason for hiding this comment

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

It is true if we mantain 7 delay days between request and effectiveness account's deletion. If we enable instant delete of personal data, it should be possible. What do you think?

Copy link
Contributor

@gunzip gunzip Nov 2, 2020

Choose a reason for hiding this comment

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

I think it's true in any scenario since when you create the (un)subscription record in the same day, you delete any previous (un)subscription here: https://github.com/pagopa/io-functions-app/blob/master/UpdateSubscriptionsFeedActivity/index.ts#L102

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8857930

@AleDore
Copy link
Contributor Author

AleDore commented Nov 2, 2020

may we add some tests?

Ok, it seems that no tests have been written for this API. Let's integrate them!

@AleDore
Copy link
Contributor Author

AleDore commented Nov 3, 2020

may we add some tests?

Ok, it seems that no tests have been written for this API. Let's integrate them!

@gunzip a04e2b1 :)

Copy link
Contributor

@balanza balanza left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 12 to 20
const today = new Date();
const tomorrow = new Date(today);
tomorrow.setDate(tomorrow.getDate() + 1);

const yesterday = new Date(today);
yesterday.setDate(yesterday.getDate() - 1);
const aServiceId = "MyServiceId" as ServiceId;

const yesterdayUTC = dateFmt.format(yesterday, "YYYY-MM-DD");
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed this: https://date-fns.org/v1.28.5/docs/startOfYesterday maybe can help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍 047a2f4

Copy link
Contributor

@gunzip gunzip left a comment

Choose a reason for hiding this comment

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

well done!

Comment on lines 12 to 20
const today = new Date();
const tomorrow = new Date(today);
tomorrow.setDate(tomorrow.getDate() + 1);

const yesterday = new Date(today);
yesterday.setDate(yesterday.getDate() - 1);
const aServiceId = "MyServiceId" as ServiceId;

const yesterdayUTC = dateFmt.format(yesterday, "YYYY-MM-DD");
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed this: https://date-fns.org/v1.28.5/docs/startOfYesterday maybe can help

@AleDore AleDore merged commit 5206ada into master Nov 3, 2020
@AleDore AleDore deleted the 175497433_add_profile_unsubscription_on_feed branch November 3, 2020 11:14
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.

4 participants