-
Notifications
You must be signed in to change notification settings - Fork 499
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
Fix #1390: Show terms of service & privacy policy notices. #3852
Conversation
@veena14cs I left feedback in chat. Please assign back or ping me if there's anything else you need my thoughts on. |
Thanks @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. |
Thanks @veena14cs! I think I'll need to take a look at this tomorrow my time, but in the meantime:
|
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? |
1- Mobile-landscape and tablet portrait + landscape mocks are not yet provided. I have included landscape screenshots though. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veena14cs I have left comments on overall approach PTAL
app/src/main/java/org/oppia/android/app/onboarding/OnboardingFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
...va/org/oppia/android/app/privacypolicytermsofservice/PrivacyPolicySingleActivityPresenter.kt
Outdated
Show resolved
Hide resolved
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @veena14cs. Took a first pass on nearly everything (skipped verifying the policy text for now, the new html utility, and tests).
My main feedback is that I think we should introduce just a single activity/fragment + presenters for both policies and make them generic. It seems both are basically identical, so the extra boilerplate seems unnecessary. Sharing them will reduce the overall surface being introduced here.
app/src/main/java/org/oppia/android/app/help/HelpActivityPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/help/HelpActivityPresenter.kt
Outdated
Show resolved
Hide resolved
@BenHenning I have fixed testcases related to canvas. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @veena14cs! Just had 4 small remaining comments, otherwise the PR LGTM!
app/src/sharedTest/java/org/oppia/android/app/parser/ListItemLeadingMarginSpanTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/parser/ListItemLeadingMarginSpanTest.kt
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/parser/ListItemLeadingMarginSpanTest.kt
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/parser/ListItemLeadingMarginSpanTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @veena14cs. Just a couple more follow-up comments--PTAL.
app/src/sharedTest/java/org/oppia/android/app/parser/ListItemLeadingMarginSpanTest.kt
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/parser/ListItemLeadingMarginSpanTest.kt
Outdated
Show resolved
Hide resolved
I have addressed and fixed the comments. PTAL. |
Thanks @veena14cs! I have no further comments, so I think we're mainly waiting on CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @veena14cs! Everything here LGTM! This was a set of gnarly problems, and it's really nice to see all of this come together.
Explanation
Fix #1390
This PR fixes #1390. 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
..............
.....
.....
.....
......
....
.....
Tablet :
....
....
.........
.....
.....
Screenshots on Mdpi device
....
......
...
LTR and RTL list Tablet
......
....
...
....
LTR Video
Screenrecording_20220309_155903.mp4
RTL Video
Screenrecording_20220804_111917.mp4
Essential Checklist
Known Issues