Skip to content

Remove idv/personal_key#download action#6162

Merged
zachmargolis merged 1 commit intomainfrom
margolis-remove-download-action
Apr 6, 2022
Merged

Remove idv/personal_key#download action#6162
zachmargolis merged 1 commit intomainfrom
margolis-remove-download-action

Conversation

@zachmargolis
Copy link
Contributor

Why: Unused as of #6161

changelog: Internal, Source code, Remove unused download code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌶️ one downside to not having a server action is we no longer get the analytics

However, I still feel strongly enough that having this action on the server is a liability, so I am OK not having analytics if it means we can have a more bug-free frontend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We have some precedent for front-end logging with #5875, which might be useful for this.

(Also commented as such on related ticket)

Base automatically changed from margolis-download-personal-key to main April 5, 2022 22:29
**Why**: Unused as of #6161

changelog: Internal, Source code, Remove unused download code
@zachmargolis zachmargolis force-pushed the margolis-remove-download-action branch from b4ea3b6 to e473b59 Compare April 5, 2022 22:40
Comment on lines +182 to +183
# @deprecated
# A user has downloaded their personal key. This event is no longer emitted.
Copy link
Contributor

Choose a reason for hiding this comment

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

YAGNI?

Or is this just so that we have a reference in the analytics documentation for historical lookups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I figured it would be good to have a reference. And if we choose to implement frontend logging, we can link these two as "previous event names" for that or something

@zachmargolis
Copy link
Contributor Author

using admin merge to override code coverage dropping

@zachmargolis zachmargolis merged commit b2615d2 into main Apr 6, 2022
@zachmargolis zachmargolis deleted the margolis-remove-download-action branch April 6, 2022 17: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.

3 participants