Fix #3485: Added GaeModels along with PlatformParameterService and its Mock #3489

Arjupta · 2021-07-17T03:42:22Z

Explanation

Fixes #3485
PlatformParameter API & Gae Models for hooking up the backend and including Mock Platform Parameter Service for representing fake network response.

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.

…Test to data

…to new file

…esting/platformparameter BUILD.bazel

jcqli

Looks pretty good, left a few comments. Otherwise, be sure to include those data files in the test exemption file if we won't have tests for them.

data/src/main/java/org/oppia/android/data/backends/gae/model/GaePlatformParameters.kt

jcqli · 2021-07-20T00:31:11Z

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt

+/** Service that provides access to platform parameter endpoint. */
+interface PlatformParameterService {
+
+// TODO("Change the url to point to the correct endpoint when the backend is ready")


Is there no backend for this yet? I had thought the backend was done in a previous project.

There is: https://github.com/oppia/oppia/blob/develop/core/controllers/platform_feature.py#L28.

Or, is this blocking on updating that API? (in which case, shouldn't that happen first?)

@BenHenning and @jcqli currently we have an endpoint in Oppia for platform parameters but making any changes to it was discussed to be put in the Future Works in the Proposal. Thats why I have mentioned this as TODO becuase we have not confirmed that whether we will be sending version number to the backend or the names of paramaters to be returned in the response. //cc: @vinitamurthi

Ack, make an issue for it an include the issue # here then.

This actually seems concerning to me. We shouldn't be setting up the model layer for an endpoint format that doesn't exist. Either these API endpoint representations should match the current backend, or the current backend should be updated to fit these. Otherwise, the system isn't going to work. 'Future work' should be optional, not required, pieces of work beyond the defined requirements for the system to work within spec.

oppiabot · 2021-07-20T00:40:45Z

Unassigning @jcqli since the review is done.

oppiabot · 2021-07-20T00:40:48Z

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

BenHenning

Thanks @Arjupta! PTAL at my comments. Also, you have a failing static check--could you please take a look & address that?

Thanks!

data/src/main/java/org/oppia/android/data/backends/gae/NetworkModule.kt

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt

BenHenning · 2021-07-20T05:59:35Z

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt

+/** Service that provides access to platform parameter endpoint. */
+interface PlatformParameterService {
+
+// TODO("Change the url to point to the correct endpoint when the backend is ready")


There is: https://github.com/oppia/oppia/blob/develop/core/controllers/platform_feature.py#L28.

Or, is this blocking on updating that API? (in which case, shouldn't that happen first?)

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt

data/src/main/java/org/oppia/android/data/backends/gae/model/GaePlatformParameters.kt

data/src/test/java/org/oppia/android/data/backends/gae/api/PlatformParameterServiceTest.kt

testing/src/main/java/org/oppia/android/testing/network/MockPlatformParameterService.kt

.../src/main/java/org/oppia/android/testing/platformparameter/TestPlatformParameterConstants.kt

…and Gae models also adding test cases in Service test and documentation over Gae models

jcqli

Overall LGTM, although there is a build error that I'm not familiar with.. couldn't find anything concrete about it online so I re-ran the checks. Recommend keeping an eye on it.

jcqli · 2021-07-20T18:28:33Z

Overall LGTM, although there is a build error that I'm not familiar with.. couldn't find anything concrete about it online so I re-ran the checks. Recommend keeping an eye on it.

I saw your message that this might be an issue with Bazel & CI. Noting here it might not be resolvable in this PR.

anandwana001

Tests LGTM

and KDoc changes

BenHenning

Thanks @Arjupta. Had a few nits, but my main feedback is that I think we still need at least a basic test suite for the new mock especially since it'll probably become more complex in subsequent PRs & in the future.

Also, apologies for not reviewing this earlier. I think it slipped off my radar this week somehow.

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt

...ing/src/main/java/org/oppia/android/testing/platformparameter/TestStringPlatformParameter.kt

testing/src/main/java/org/oppia/android/testing/platformparameter/BUILD.bazel

data/src/test/java/org/oppia/android/data/backends/gae/api/PlatformParameterServiceTest.kt

testing/src/main/java/org/oppia/android/testing/network/MockPlatformParameterService.kt

BenHenning

Thanks @Arjupta. Were all of my previous comments addressed? Some of the comment threads didn't have any replies, and don't seem to correspond to recent changes.

testing/BUILD.bazel

BenHenning

Thanks @Arjupta! Just a couple a super small nits left.

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt

BenHenning

Thanks @Arjupta. LGTM!

Added GaeModels + PlatformParameterService and its Mock

405fc44

Arjupta added this to the GSoC 2021.4: Milestone 2 milestone Jul 17, 2021

Arjupta requested review from anandwana001 and BenHenning as code owners July 17, 2021 03:42

Arjupta added this to In progress in GSoC 2021.4: Feature Flags & Platform Parameters Jul 17, 2021

Arjupta added 6 commits July 17, 2021 20:50

Trying to fix bazel check by adding dependency over data module

c51a184

Changed the location of MockService to testing/network and of Service…

d7a01c5

…Test to data

Bazel changes for TestConstants alomng with moving these constants in…

b2aa553

…to new file

Nit change in the bazel package name

3de99ed

Adding platform parameter BUILD.bazel as a dependency in testing

cc6e47a

Adding test_constants in testing/network BUILD.bazel and utility in t…

1879820

…esting/platformparameter BUILD.bazel

Arjupta assigned vinitamurthi, BenHenning, anandwana001 and jcqli Jul 19, 2021

jcqli suggested changes Jul 20, 2021

View reviewed changes

oppiabot bot unassigned jcqli Jul 20, 2021

oppiabot bot assigned Arjupta Jul 20, 2021

BenHenning requested changes Jul 20, 2021

View reviewed changes

BenHenning removed their assignment Jul 20, 2021

Arjupta added 2 commits July 20, 2021 22:31

Suggested changes related to exemption file, renaming test constants …

44dabec

…and Gae models also adding test cases in Service test and documentation over Gae models

Nit change related to TODO issue number

ffcc93a

Arjupta assigned BenHenning and jcqli and unassigned Arjupta Jul 20, 2021

jcqli approved these changes Jul 20, 2021

View reviewed changes

oppiabot bot unassigned jcqli Jul 20, 2021

BenHenning assigned Arjupta Jul 23, 2021

anandwana001 approved these changes Jul 24, 2021

View reviewed changes

anandwana001 removed their assignment Jul 24, 2021

Arjupta added 2 commits July 24, 2021 17:19

Suggested change for testing request query, new files for test constants

f59305d

and KDoc changes

Static checks and updating exemption file

c642b47

Arjupta assigned BenHenning and unassigned Arjupta Jul 25, 2021

Updating with develop

d354c05

Arjupta mentioned this pull request Jul 28, 2021

Fix #3605: Introduce Platform Parameter WorkManager classes #3606

Merged

8 tasks

BenHenning requested changes Jul 30, 2021

View reviewed changes

BenHenning assigned Arjupta and unassigned BenHenning Jul 30, 2021

Sparsh1212 added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Jul 30, 2021

Arjupta added 3 commits August 2, 2021 01:09

Added tests for MockPlatformParameterService

af760fa

Updating with develop

6f1efe7

Added test files in the kdoc-exemption

4fa3761

Arjupta removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Aug 3, 2021

Arjupta assigned BenHenning and unassigned Arjupta Aug 3, 2021

BenHenning requested changes Aug 4, 2021

View reviewed changes

testing/BUILD.bazel Outdated Show resolved Hide resolved

BenHenning assigned Arjupta and unassigned BenHenning Aug 4, 2021

Nit changes related to KDoc and BUILD.bazel

54c595b

BenHenning reviewed Aug 4, 2021

View reviewed changes

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt Outdated Show resolved Hide resolved

data/src/main/java/org/oppia/android/data/backends/gae/api/PlatformParameterService.kt Outdated Show resolved Hide resolved

Nit change of Reference Url in PlatformParameterService

c02a0ec

BenHenning approved these changes Aug 4, 2021

View reviewed changes

BenHenning enabled auto-merge (squash) August 4, 2021 10:32

BenHenning merged commit 9c5feef into develop Aug 4, 2021

BenHenning deleted the platform-parameter-hooking-backend branch August 4, 2021 11:09

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3485: Added GaeModels along with PlatformParameterService and its Mock #3489

Fix #3485: Added GaeModels along with PlatformParameterService and its Mock #3489

Arjupta commented Jul 17, 2021

jcqli left a comment •

edited

jcqli Jul 20, 2021

BenHenning Jul 20, 2021

Arjupta Jul 20, 2021 •

edited

jcqli Jul 20, 2021

Arjupta Jul 20, 2021

BenHenning Jul 21, 2021 •

edited

oppiabot bot commented Jul 20, 2021

oppiabot bot commented Jul 20, 2021

BenHenning left a comment

BenHenning Jul 20, 2021

jcqli left a comment

jcqli commented Jul 20, 2021

anandwana001 left a comment

BenHenning left a comment •

edited

BenHenning left a comment

BenHenning left a comment

BenHenning left a comment

Fix #3485: Added GaeModels along with PlatformParameterService and its Mock #3489

Fix #3485: Added GaeModels along with PlatformParameterService and its Mock #3489

Conversation

Arjupta commented Jul 17, 2021

Explanation

Checklist

jcqli left a comment • edited

Choose a reason for hiding this comment

jcqli Jul 20, 2021

Choose a reason for hiding this comment

BenHenning Jul 20, 2021

Choose a reason for hiding this comment

Arjupta Jul 20, 2021 • edited

Choose a reason for hiding this comment

jcqli Jul 20, 2021

Choose a reason for hiding this comment

Arjupta Jul 20, 2021

Choose a reason for hiding this comment

BenHenning Jul 21, 2021 • edited

Choose a reason for hiding this comment

oppiabot bot commented Jul 20, 2021

oppiabot bot commented Jul 20, 2021

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Jul 20, 2021

Choose a reason for hiding this comment

jcqli left a comment

Choose a reason for hiding this comment

jcqli commented Jul 20, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

BenHenning left a comment • edited

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

jcqli left a comment •

edited

Arjupta Jul 20, 2021 •

edited

BenHenning Jul 21, 2021 •

edited

BenHenning left a comment •

edited