oppia-android icon indicating copy to clipboard operation
oppia-android copied to clipboard

Published 20 hours ago • verified publisher icon oppia

Fix #1390, #1391: Show terms of service & privacy policy notices.

Open veena14cs opened this issue 3 years ago • 60 comments

Explanation

Fix #1390 Fix #1391

This PR fixes #1390 & #1391. This PR introduces Privacy Policy and Terms of Service in the app. The PR contains Policies page through onboarding flow and FAQ/Help screens.

This PR also fixes #1022. This PR introduces two new custom tags <oppia-ul> and <oppia-ol> to support html <ul> and <ol> tags respectively. To addon the new custom list tags also supports lower versions devices and nested list tags.

This PR also fixes #2193 RTL issues in Bullet points and fixes the crashes in the issue #2391

Mock Links

Onboarding Mobile Landscape Tablet FAQ and Help Mobile Landscape Tablet

Document Reference

https://docs.google.com/document/d/1QoW2S-HGNrQ_6rodOByjGPd8e_w80PHKE3PJ82t1-hE/edit?usp=sharing

Screenshots

Mobile Portrait on Nexus 6

Screenshot_1634630041.....Screenshot_1634949081...Screenshot_1634630033......Screenshot_1634949088

Screenshot_1637266595.....Screenshot_1637266617

Screenshot_1637266577.....Screenshot_1637266625

Screenshot_1634630045.....Screenshot_1634949092

Screenshot_1634948852......Screenshot_1634949016

Screenshot_1634948847....Screenshot_1634949022

Screenshot_1634948856.....Screenshot_1634949010

Tablet :

Screenshot_1635233546....Screenshot_1635233563

Screenshot_1635233541....Screenshot_1635233511

Screenshot_1635233504....Screenshot_1635233521.....Screenshot_1635233492

Screenshot_1642254470.....Screenshot_1642254531

Screenshot_1642254476.....Screenshot_1642254522

Screenshots on Mdpi device

Screenshot_1645212105....Screenshot_1645212130 Screenshot_1659367710......Screenshot_1659367631 Screenshot_1659367721...Screenshot_1659367673

LTR and RTL list Tablet

Screenshot_1659368858......Screenshot_1659368794 Screenshot_1659368868....Screenshot_1659368768

Screenshot_1659368880...Screenshot_1659368682 Screenshot_1659368875....Screenshot_1659368747

LTR Video

https://user-images.githubusercontent.com/16301028/158009697-a64cb7dd-37e3-4130-aae4-85a9f3e942a7.mp4

RTL Video

https://user-images.githubusercontent.com/16301028/182775534-a4ec3173-9e30-4ee0-a532-a28d99bc76a6.mp4

Essential Checklist

  • [x] 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: ...".)
  • [x] Any changes to scripts/assets files have their rationale included in the PR explanation.
  • [x] The PR follows the style guide.
  • [x] The PR does not contain any unnecessary code changes from Android Studio (reference).
  • [x] The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • [x] The PR is assigned to the appropriate reviewers (reference).

Known Issues

  • New custom List tag doesn't support RTL

veena14cs avatar Sep 28 '21 09:09 veena14cs

@veena14cs I left feedback in chat. Please assign back or ping me if there's anything else you need my thoughts on.

BenHenning avatar Oct 05 '21 03:10 BenHenning

@veena14cs I left feedback in chat. Please assign back or ping me if there's anything else you need my thoughts on.

Thanks @BenHenning .

veena14cs avatar Oct 05 '21 05:10 veena14cs

Hi @veena14cs, 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[bot] avatar Oct 13 '21 11:10 oppiabot[bot]

Thanks @veena14cs! I think I'll need to take a look at this tomorrow my time, but in the meantime:

  1. Can you add screenshots for landscape for all the mocks currently in the opening comment to show how landscape works? Ditto for tablet portrait + landscape.
  2. We need to include links to the web copies of the policies. Are these included? Can you maybe include a video showing navigating to each policy & clicking on the links?
  3. Can you please add a video showing the TalkBack experience for each screen?
  4. CI has failures--can you make sure that these are fixed?

BenHenning avatar Oct 21 '21 05:10 BenHenning

Sorry, will need to review this next week. Given that, @veena14cs can you please address all the points in https://github.com/oppia/oppia-android/pull/3852#issuecomment-948281355 & assign back one done?

BenHenning avatar Oct 22 '21 08:10 BenHenning

Thanks @veena14cs! I think I'll need to take a look at this tomorrow my time, but in the meantime:

  1. Can you add screenshots for landscape for all the mocks currently in the opening comment to show how landscape works? Ditto for tablet portrait + landscape.
  2. We need to include links to the web copies of the policies. Are these included? Can you maybe include a video showing navigating to each policy & clicking on the links?
  3. Can you please add a video showing the TalkBack experience for each screen?
  4. CI has failures--can you make sure that these are fixed?

1- Mobile-landscape and tablet portrait + landscape mocks are not yet provided. I have included landscape screenshots though. 2- I have added link at the bottom of the page for both PP/Tos PTAL in the video. 3- I will have to check on this still. 4- Waiting for the tests to complete.

veena14cs avatar Oct 22 '21 19:10 veena14cs

Sorry, will need to review this next week. Given that, @veena14cs can you please address all the points in #3852 (comment) & assign back one done?

Thanks for the update @BenHenning . Can you please look at the video once and confirm if its correct. I have added web link for PP/Tos on the Privacy policy and Terms of service page.

veena14cs avatar Oct 22 '21 19:10 veena14cs

Sorry, will need to review this next week. Given that, @veena14cs can you please address all the points in #3852 (comment) & assign back one done?

Thanks for the update @BenHenning . Can you please look at the video once and confirm if its correct. I have added web link for PP/Tos on the Privacy policy and Terms of service page.

@veena14cs the videos are flickering really badly and it's making it difficult to evaluate. Can you maybe try to re-record them or check with someone else to see if it works for them? It may be specific to my computer, but I haven't had this issue with other uploaded videos.

Beyond that, I plan to look at this PR in more depth tomorrow.

BenHenning avatar Oct 26 '21 05:10 BenHenning

@veena14cs I will have a look at this tomorrow. Thanks.

rt4914 avatar Nov 03 '21 21:11 rt4914

@BenHenning and @rt4914 PTAL. Please ignore check failures for now as I want to finalize the implementation and then add Tests for it.

veena14cs avatar Nov 18 '21 20:11 veena14cs

FYI I'm listed as a codeowner for this PR & I'll be unavailable to perform code reviews over the next 2 weeks--thanks for your patience.

BenHenning avatar Nov 20 '21 00:11 BenHenning

@veena14cs I won't be able to perform a full review on this until I return from my break, but after a quick glance I think it looks pretty solid from an approach perspective. Since it's mostly UI, and you incorporated my main feedback (i.e. using protos for passing around intent/fragment args & using a custom tag handler), I defer to @rt4914 to make the final call on whether the approach looks good to unblock writing tests. I'll review the whole PR when I get back.

BenHenning avatar Nov 20 '21 07:11 BenHenning

@veena14cs The approach does look good. You can write test cases. PTAL at #3852 (review)

Thanks @rt4914 will be following up with Testcases and assign you back.

veena14cs avatar Nov 24 '21 18:11 veena14cs

Unassigning @veena14cs since a re-review was requested. @veena14cs, please make sure you have addressed all review comments. Thanks!

oppiabot[bot] avatar Nov 24 '21 18:11 oppiabot[bot]

As per last comment it seems oppia-bot assigned this incorrectly and therefore un-assigning myself.

rt4914 avatar Nov 24 '21 19:11 rt4914

Assigning @vinitamurthi, @BenHenning for code owner reviews. Thanks!

oppiabot[bot] avatar Dec 01 '21 19:12 oppiabot[bot]

@vinitamurthi there is a issue with enum PolicyPage. I am getting error as unreferenced PolicyPage in ci tests, where as in Android studio it is not giving any error. Do I need to have bazel build file for this? Can you please check it. Also I have addressed your comment and made changes PTAL.

@rt4914 I have made some changes after your review, there was issue with PolicyPageTagHandler in RTL mode, which is been corrected now, can you please review once again.

veena14cs avatar Dec 03 '21 18:12 veena14cs

Hi. FYI I've been out the last couple of weeks, so I'm working to catch up on my reviews. I might be delayed a couple of days, but I'll be reviewing this soon. Thanks for your patience!

BenHenning avatar Dec 07 '21 03:12 BenHenning

@vinitamurthi there is a issue with enum PolicyPage. I am getting error as unreferenced PolicyPage in ci tests, where as in Android studio it is not giving any error. Do I need to have bazel build file for this? Can you please check it. Also I have addressed your comment and made changes PTAL.

@rt4914 I have made some changes after your review, there was issue with PolicyPageTagHandler in RTL mode, which is been corrected now, can you please review once again.

@veena14cs The change for PolicyPageTagHandler makes sense but now there are test cases failing in StateFragmentLocalTest can you please check this. I think the changes in utility module are affecting this.

rt4914 avatar Dec 07 '21 04:12 rt4914

@veena14cs The change for PolicyPageTagHandler makes sense but now there are test cases failing in StateFragmentLocalTest can you please check this. I think the changes in utility module are affecting this.

@rt4914 I am not able to figure out this issue. Is is something related to Bazel build?

veena14cs avatar Dec 07 '21 18:12 veena14cs

@BenHenning Can you please look at the CI failures, there are two types required failures (1.) /home/runner/.cache/bazel/_bazel_runner/ef69b31fa14ea8341d6589b0a8832bdd/execroot/__main__/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt:12:36: error: unresolved reference: PolicyPage import org.oppia.android.app.model.PolicyPage (2.) All test fo StateFragmentLocalTest are failing and as per my discussion with @veena14cs these tests are passing locally.

UPDATE: The tests are not passing locally too. I will check this on my side first.

rt4914 avatar Dec 08 '21 19:12 rt4914

@BenHenning Can you please look at the CI failures, there are two types required failures (1.) /home/runner/.cache/bazel/_bazel_runner/ef69b31fa14ea8341d6589b0a8832bdd/execroot/__main__/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt:12:36: error: unresolved reference: PolicyPage import org.oppia.android.app.model.PolicyPage (2.) All test fo StateFragmentLocalTest are failing and as per my discussion with @veena14cs these tests are passing locally.

UPDATE: The tests are not passing locally too. I will check this on my side first.

@rt4914 @veena14cs this is because HtmlParser was updated to reference a proto file, but the utility package wasn't updated to depend on the proto file: https://github.com/oppia/oppia-android/blob/develop/utility/BUILD.bazel#L46. That needs to be updated to include https://github.com/oppia/oppia-android/blob/develop/model/BUILD.bazel#L48 in its deps list.

BenHenning avatar Dec 11 '21 04:12 BenHenning

@BenHenning Can you please look at the CI failures, there are two types required failures (1.) /home/runner/.cache/bazel/_bazel_runner/ef69b31fa14ea8341d6589b0a8832bdd/execroot/__main__/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt:12:36: error: unresolved reference: PolicyPage import org.oppia.android.app.model.PolicyPage (2.) All test fo StateFragmentLocalTest are failing and as per my discussion with @veena14cs these tests are passing locally. UPDATE: The tests are not passing locally too. I will check this on my side first.

@rt4914 @veena14cs this is because HtmlParser was updated to reference a proto file, but the utility package wasn't updated to depend on the proto file: https://github.com/oppia/oppia-android/blob/develop/utility/BUILD.bazel#L46. That needs to be updated to include https://github.com/oppia/oppia-android/blob/develop/model/BUILD.bazel#L48 in its deps list.

To include it in the above files, first I need to create BUILD.bazel file for HtmlParser?

veena14cs avatar Dec 11 '21 09:12 veena14cs

@vinitamurthi there is a issue with enum PolicyPage. I am getting error as unreferenced PolicyPage in ci tests, where as in Android studio it is not giving any error. Do I need to have bazel build file for this? Can you please check it. Also I have addressed your comment and made changes PTAL.

@rt4914 I have made some changes after your review, there was issue with PolicyPageTagHandler in RTL mode, which is been corrected now, can you please review once again.

Changes LGTM, you may have to add the bazel build dependency for this proto if the error is still happening

vinitamurthi avatar Dec 12 '21 02:12 vinitamurthi

Assigning @rt4914 for code owner reviews. Thanks!

oppiabot[bot] avatar Dec 12 '21 02:12 oppiabot[bot]

@BenHenning Can you please look at the CI failures, there are two types required failures (1.) /home/runner/.cache/bazel/_bazel_runner/ef69b31fa14ea8341d6589b0a8832bdd/execroot/__main__/utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt:12:36: error: unresolved reference: PolicyPage import org.oppia.android.app.model.PolicyPage (2.) All test fo StateFragmentLocalTest are failing and as per my discussion with @veena14cs these tests are passing locally. UPDATE: The tests are not passing locally too. I will check this on my side first.

@rt4914 @veena14cs this is because HtmlParser was updated to reference a proto file, but the utility package wasn't updated to depend on the proto file: https://github.com/oppia/oppia-android/blob/develop/utility/BUILD.bazel#L46. That needs to be updated to include https://github.com/oppia/oppia-android/blob/develop/model/BUILD.bazel#L48 in its deps list.

To include it in the above files, first I need to create BUILD.bazel file for HtmlParser?

Nope, you just need to update the Bazel library that HtmlParser is included in (which I linked to above--it's part of the "utility" library).

BenHenning avatar Dec 14 '21 05:12 BenHenning

Hi. As of today, some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 4 January 2021.

BenHenning avatar Dec 17 '21 05:12 BenHenning

Hi @veena14cs, 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[bot] avatar Dec 28 '21 10:12 oppiabot[bot]

Hi @veena14cs, 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[bot] avatar Jan 09 '22 18:01 oppiabot[bot]

@veena14cs I'm assigning this back to you until the RTL issue with links can be resolved. Please assign back once that's completed & the PR has any necessary updates.

BenHenning avatar Jan 15 '22 00:01 BenHenning