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.

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

Legals #4660

Merged
merged 15 commits into from
Dec 10, 2021
Merged

Legals #4660

merged 15 commits into from
Dec 10, 2021

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Dec 8, 2021

Implement the first item of #4594

New Legals section in the main setting menu:

image

@amshakal which icon can I used for this item?
EDIT: icon updated

Legals screen

  • App policies (was previously on Help & about screen)
  • homeserver policies (new)
  • identity server policies, if there is one configured (else this section is not displayed). Same info displayed than on the discovery screen (which will have to be updated later, it's quite confusing today, there is a mix between identity server configuration and discovery settings)
  • third party libraries (was previously on Help & about screen)

image

Help and About screen updated

With a new help section. Fixes #4638

Before After When help is clicked
image image image

@Suppress("UNCHECKED_CAST")
((throwable.toRegistrationFlowResponse()
?.params?.get(LoginFlowTypes.TERMS) as? JsonDict)
?.get("policies") as? JsonDict)
Copy link
Member Author

Choose a reason for hiding this comment

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

A bit dirty but it works...

@bmarty bmarty marked this pull request as ready for review December 8, 2021 22:44
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

Unit Test Results

  66 files  ±0    66 suites  ±0   59s ⏱️ -1s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit bf7907a. ± Comparison against base commit 8fefee9.

♻️ This comment has been updated with latest results.

@amshakal
Copy link

amshakal commented Dec 9, 2021

@bmarty I think we can use this icon for it.
Legal_Files

private suspend fun getHomeserverTerms(baseUrl: String): GetTermsResponse {
return try {
executeRequest(null) {
termsAPI.register(baseUrl + NetworkConstants.URI_API_PREFIX_PATH_R0 + "register")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this something we should consider adding to the spec? it looks like we're relying on the register endpoint to fail

Copy link
Member Author

Choose a reason for hiding this comment

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

the register endpoint will fail for sure, we are not providing any data to the request. Also if registration is disabled, I'm not sure what will happen.
This is a temporary workaround waiting for matrix-org/matrix-spec-proposals#3012 to land

/**
* It's only possible to use this value for [getTerms]
*/
Homeserver
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it's worth having a separate getHomeserverTerms to avoid potential misunderstandings with this api?

Copy link
Member Author

Choose a reason for hiding this comment

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

I though about it, will iterate.

Copy link
Member Author

Choose a reason for hiding this comment

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

class ElementLegals @Inject constructor(
private val stringProvider: StringProvider
) {
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: I don't think this comment/doc adds much, do you think it's needed? renaming getData to getServerPolicies would avoid the need for a 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.

Probably but a bit more confusing, since there is no server involved here...

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

awesome stuff! 💯 hopefully this helps reduce our google play rejections

@bmarty bmarty enabled auto-merge December 9, 2021 11:26
@bmarty bmarty force-pushed the feature/bma/legals branch from 49721ed to bf7907a Compare December 10, 2021 14:35
@bmarty
Copy link
Member Author

bmarty commented Dec 10, 2021

Force-pushed to fix merge conflict in VectorPreferences

@bmarty bmarty merged commit 34898f1 into develop Dec 10, 2021
@bmarty bmarty deleted the feature/bma/legals branch December 10, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When a user clicks "Help & About" they only see About
3 participants