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

ALTAPPS-1323: Shared research analytic logging issue #1145

Conversation

XanderZhu
Copy link
Contributor

@XanderZhu XanderZhu commented Aug 2, 2024

YouTrack Issues:
ALTAPPS-1323

Checklist

Before Code Review:

  • Fields "Assignees, Labels, Milestone" are filled in the pull request;
  • All checks have been passed;
  • Changes have been checked locally

Description

  • Rename AnalyticActionDispatcher to CompletableCoroutineActionDispatcher and remove analytic reference;
  • Add CompletableCoroutineActionDispatcherTest;
  • Return events to the cache in case of failed flush request;
  • Add AnalyticHyperskillRepositoryTest.

@XanderZhu XanderZhu self-assigned this Aug 2, 2024
@XanderZhu XanderZhu added the shared Shared module task label Aug 2, 2024
@XanderZhu XanderZhu marked this pull request as ready for review August 6, 2024 09:34
@XanderZhu XanderZhu added the awaiting review Pull Request is awaiting code reviews label Aug 6, 2024
@github-actions github-actions bot added the android Android module task label Aug 6, 2024
@ivan-magda ivan-magda added this to the 1.67 milestone Aug 6, 2024
.flushEvents(eventsToFlush, isAuthorized)
.onFailure {
mutex.withLock {
hyperskillCacheDataSource.logEvents(eventsToFlush)
Copy link
Member

Choose a reason for hiding this comment

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

I suppose that there is a case where the current user profile can be changed between logEvent, and old analytic events can revert to being associated with the previous user profile.
However, it is not so critical right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivan-magda
Yes, you are right. Good point to think about.

@ivan-magda ivan-magda added ready to pull Pull Request is ready to merge and removed awaiting review Pull Request is awaiting code reviews labels Aug 6, 2024
@ivan-magda ivan-magda merged commit c4be852 into develop Aug 7, 2024
13 checks passed
@ivan-magda ivan-magda deleted the feature/ALTAPPS-1323/Shared-research-analytic-logging-issue branch August 7, 2024 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Android module task ready to pull Pull Request is ready to merge shared Shared module task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants