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.

Sign up for GitHub

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

Jump to bottom

Fixes #4405: Renaming event logger to analytics event logger #4537

Closed
wants to merge 13 commits into from
Closed

Fixes #4405: Renaming event logger to analytics event logger #4537

wants to merge 13 commits into from

Conversation

Akshatkamboj14
Copy link
Member

Explanation

Fixes #4405

Potential checklist:

  • changes in all the production files where eventLogger is used,
  • changes in test files.
  • changes in dagger modules.
  • refactoring of fakeEventlogger to fakeAnalyticsEventLogger

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

rt4914
rt4914 suggested changes Aug 29, 2022
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@Akshatkamboj14 PTAL. Thanks

app/src/main/java/org/oppia/android/app/devoptions/vieweventlogs/ViewEventLogsViewModel.kt Outdated
@@ -13,7 +13,7 @@ import javax.inject.Inject
*/
@FragmentScope
class ViewEventLogsViewModel @Inject constructor(
debugEventLogger: DebugEventLogger,
debugEventLogger: DebugAnalyticsEventLogger,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename debugEventLogger to debugAnalyticsEventLogger.

Same applies for all other files too.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@rt4914 rt4914 removed their assignment Aug 29, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 29, 2022

Hi @Akshatkamboj14, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

BenHenning
BenHenning approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @Akshatkamboj14! The PR generally LGTM, but besides a couple of comments I think that you also need to:

  • Make sure there are no TODOs in code for Renaming EventLogger to AnalyticsEventLogger #4405 that need to be removed (otherwise remove them)
  • Finish the PR description & make sure that it's fully filled out correctly (look at other PRs if you need a reference for how to fill it out for non-user facing PRs)
  • Fix the broken CI checks/tests

app/src/test/java/org/oppia/android/app/player/exploration/ExplorationActivityLocalTest.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/android/util/logging/firebase/LogReportingModule.kt Outdated
@@ -17,5 +17,7 @@ class LogReportingModule {

@Provides
@Singleton
fun provideEventLogger(factory: FirebaseEventLogger.Factory): EventLogger = factory.create()
fun provideEventLogger(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fun provideEventLogger(
fun provideFirebaseAnalyticsEventLogger(

Per my earlier comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BenHenning can you send me the link of some PRs so I can take a reference from like how to fill the description of non-user facing PRs?

Copy link
Member

Choose a reason for hiding this comment

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

@Akshatkamboj14 This is something that you can look for directly. See https://github.com/oppia/oppia-android/pulls.

@BenHenning BenHenning removed their assignment Aug 29, 2022
@Akshatkamboj14 Akshatkamboj14 requested a review from rt4914 August 30, 2022 18:48
rt4914
rt4914 approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Approved for code-owners

@rt4914 rt4914 removed their assignment Sep 1, 2022
@oppiabot oppiabot bot added the PR: LGTM label Sep 1, 2022
BenHenning
BenHenning requested changes Sep 2, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Took another pass. I actually meant to request changes last time since CI checks are still breaking, so updating my review status.

@oppiabot oppiabot bot removed the PR: LGTM label Sep 2, 2022
@oppiabot
Copy link

oppiabot bot commented Sep 2, 2022

Hi, @Akshatkamboj14, the LGTM Label has been removed because the changes were requested on this PR. Thanks!.

@Akshatkamboj14
Copy link
Member Author

@BenHenning IDK why my checks are failing. can you help me to figure it out?

@BenHenning
Copy link
Member

@BenHenning IDK why my checks are failing. can you help me to figure it out?

@Akshatkamboj14 apologies I missed this (in the future, please assign me or others if we need to take action on the PR).

Regarding the errors, what specific error are you stuck on, what have you tried, and what do you think might be wrong? This is a good situation to consider writing up a brief debugging doc (https://github.com/oppia/oppia/wiki/Debugging-Docs) and sharing it to provide more specifics--these can be really helpful to help with investigation.

@oppiabot
Copy link

oppiabot bot commented Sep 16, 2022

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 16, 2022
@oppiabot oppiabot bot closed this Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanlip seanlip seanlip left review comments

@BenHenning BenHenning BenHenning requested changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@Akshatkamboj14 Akshatkamboj14

Labels
stale Corresponds to items that haven't seen a recent update and may be automatically closed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Renaming EventLogger to AnalyticsEventLogger
5 participants
@Akshatkamboj14 @BenHenning @rt4914 @seanlip @Sarthak2601