Fix #3322: Platform parameter singleton and Generic Interface #3323

Arjupta · 2021-06-13T20:34:53Z

Explanation

Fixes #3322
This PR introduces a new domain layer Singleton class which will help with storing and providing platform parameter values. This PR also includes a Generic Interface whose implementations will be provided containing Platform Parameters. (This PR is stacked over #3269). /cc @Sarthak2601

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

…Platform Parameter project in Oppia-Android. This PR will is reffered as PR-1.1 in the Milestones

… the oneof type field in ParameterWithName

…er in their name

…latformParameterMap

jcqli

Nice job with the tests! A couple things

Try to further split up test cases so that they're only checking on thing at a time. I like to think of it as if we want to minimize the number of assertions in a test so each test can be as specific as possible.
I see that protos are included in this PR -- be sure to list only the newly-changed in the PR description in case reviewers are different.
Be sure to build & test with both Gradle & Bazel to help speedup the review process. I also forget this sometimes :)

domain/src/main/java/org/oppia/android/domain/platformparameter/PlatformParameterSingleton.kt

...n/src/test/java/org/oppia/android/domain/platformparameter/PlatformParameterSingletonTest.kt

model/BUILD.bazel

utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameter.kt

oppiabot · 2021-06-15T01:12:57Z

Unassigning @jcqli since the review is done.

oppiabot · 2021-06-15T01:13:00Z

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

…and its Impl. Also changed the tests accordingly

vinitamurthi · 2021-07-03T04:51:50Z

Note that tests are failing

Arjupta · 2021-07-03T04:52:56Z

Note that tests are failing

@vinitamurthi that test is failing due to some env reading error not because of the code changes

jcqli · 2021-07-03T06:16:56Z

utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterSingleton.kt

+
+  /**
+   * Initializes a platformParameterMap in case it is empty.
+   * @param platformParameterMap [Map<String, PlatformParameter>]


nit: add newline above KDoc @ fields (but each @ line should be next to each other).

jcqli

LGTM, just be sure to update the KDocs!

anandwana001

LGTM

BenHenning

Approved for codeowners (BUILD file changes). Left one unrelated comment for one thing I noticed.

BenHenning · 2021-07-06T00:20:56Z

...n/src/main/java/org/oppia/android/domain/platformparameter/PlatformParameterSingletonImpl.kt

+/** Singleton which helps in storing and providing Platform Parameters at runtime. */
+@Singleton
+class PlatformParameterSingletonImpl @Inject constructor() : PlatformParameterSingleton {
+  private var platformParameterMap: Map<String, PlatformParameter> = mapOf()


This could be a val with a mutable list, instead, where you clear & perform a putAll to reinitialize it versus changing the parameter itself.

oppiabot · 2021-07-06T00:24:26Z

Hi @Arjupta, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

…ED on #3323] (#3361) * Cumulative Changes from PR-1.1(#3269) and 1.2(#3323) * Introduced a new Dagger Module to provide Platform Parameters. Also a PlatformParameter Constants file to store any constants related to Platform Parameters * Nit changes * Added a FakePlatformParameterModule and a PlatformParameterModuleTest * Included a Fake Singleton approach for testing DaggerModule * Lint check + updating with base PR * Reverting .idea file * Rename functions and constants, Added new Testcases for different data type PlatformParameter * Nit KDoc changes * Changing the Testing approach + Updating with Base PR * Nit changes + New Test Case for Partial Map * Changed the location of TestSpecific constants to Fake Module * Renaming functions and removing redundant references

Arjupta added 9 commits June 7, 2021 15:03

This PR introduces new Proto Classes which will be used to implement …

b085c41

…Platform Parameter project in Oppia-Android. This PR will is reffered as PR-1.1 in the Milestones

Lint check correction

83f7330

Removing unnecessary spaces

8c0787c

Changed the name of Parameter class to PlatformParameter and included…

36e9619

… the oneof type field in ParameterWithName

Changed the names of all the other classes to include PlatformParamet…

93761bd

…er in their name

nit space change

9608308

Reverting back to previous structure

daeae25

Suggested changes by Ben about spacing, renaming, docs and removing P…

fe63e7a

…latformParameterMap

Updating with develop

2010e17

Arjupta requested review from anandwana001 and vinitamurthi as code owners June 13, 2021 20:34

Arjupta assigned vinitamurthi and jcqli Jun 13, 2021

Arjupta changed the title ~~Fixes #3322: Platform parameter singleton~~ Fix #3322: Platform parameter singleton and Generic Interface Jun 14, 2021

Modifying BUILD.bazel to include platform_parameter.proto

c457f68

Arjupta force-pushed the platform-parameter-singleton branch from 3344b17 to 5557775 Compare June 14, 2021 16:46

Arjupta requested a review from fsharpasharp as a code owner June 14, 2021 16:46

jcqli suggested changes Jun 15, 2021

View reviewed changes

oppiabot bot unassigned jcqli Jun 15, 2021

oppiabot bot assigned Arjupta Jun 15, 2021

Arjupta force-pushed the platform-parameter-singleton branch from 5557775 to a9cbdb3 Compare June 15, 2021 08:12

Changed the visibility in BUILD.bazel

0cff205

Arjupta force-pushed the platform-parameter-singleton branch from a9cbdb3 to d9f5111 Compare June 15, 2021 11:45

Arjupta assigned fsharpasharp and unassigned vinitamurthi and Arjupta Jun 15, 2021

anandwana001 assigned Arjupta and unassigned anandwana001 Jul 2, 2021

Arjupta assigned anandwana001 and unassigned Arjupta Jul 2, 2021

Changed the approach to have an PlatformParameterSingleton interface …

22340e1

…and its Impl. Also changed the tests accordingly

Arjupta assigned vinitamurthi and jcqli Jul 2, 2021

vinitamurthi approved these changes Jul 3, 2021

View reviewed changes

vinitamurthi removed their assignment Jul 3, 2021

jcqli reviewed Jul 3, 2021

View reviewed changes

jcqli approved these changes Jul 3, 2021

View reviewed changes

jcqli assigned Arjupta and unassigned jcqli Jul 3, 2021

anandwana001 approved these changes Jul 3, 2021

View reviewed changes

anandwana001 removed their assignment Jul 3, 2021

Nit spacing changes

75afce0

Arjupta removed their assignment Jul 5, 2021

BenHenning approved these changes Jul 6, 2021

View reviewed changes

BenHenning assigned Arjupta and unassigned BenHenning Jul 6, 2021

oppiabot bot added the PR: LGTM label Jul 6, 2021

Using mutableMap instead for PlatformParameterMap in Singleton

5f4a283

Arjupta removed their assignment Jul 6, 2021

Updating with develop

4e8bcdf

vinitamurthi merged commit a237d00 into oppia:develop Jul 7, 2021

Arjupta moved this from In progress to Done in GSoC 2021.4: Feature Flags & Platform Parameters Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3322: Platform parameter singleton and Generic Interface #3323

Fix #3322: Platform parameter singleton and Generic Interface #3323

Arjupta commented Jun 13, 2021 •

edited

jcqli left a comment •

edited

oppiabot bot commented Jun 15, 2021

oppiabot bot commented Jun 15, 2021

vinitamurthi commented Jul 3, 2021

Arjupta commented Jul 3, 2021

jcqli Jul 3, 2021 •

edited

Arjupta Jul 4, 2021

jcqli left a comment

anandwana001 left a comment

BenHenning left a comment

BenHenning Jul 6, 2021

Arjupta Jul 6, 2021

oppiabot bot commented Jul 6, 2021

Fix #3322: Platform parameter singleton and Generic Interface #3323

Fix #3322: Platform parameter singleton and Generic Interface #3323

Conversation

Arjupta commented Jun 13, 2021 • edited

Explanation

Checklist

jcqli left a comment • edited

Choose a reason for hiding this comment

oppiabot bot commented Jun 15, 2021

oppiabot bot commented Jun 15, 2021

vinitamurthi commented Jul 3, 2021

Arjupta commented Jul 3, 2021

jcqli Jul 3, 2021 • edited

Choose a reason for hiding this comment

Arjupta Jul 4, 2021

Choose a reason for hiding this comment

jcqli left a comment

Choose a reason for hiding this comment

anandwana001 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Jul 6, 2021

Choose a reason for hiding this comment

Arjupta Jul 6, 2021

Choose a reason for hiding this comment

oppiabot bot commented Jul 6, 2021

Arjupta commented Jun 13, 2021 •

edited

jcqli left a comment •

edited

jcqli Jul 3, 2021 •

edited