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

[MS] Adds accept tos modal #8483

Merged
merged 12 commits into from
Oct 10, 2024
Merged

[MS] Adds accept tos modal #8483

merged 12 commits into from
Oct 10, 2024

Conversation

Max-7
Copy link
Contributor

@Max-7 Max-7 commented Sep 23, 2024

Closes #7633
Depends on #8471

To test:

  1. Don't forget to recompile, python make.py i && python make.py ei
  2. Start the testbed
  3. Open electron. On the device page, note the org name
  4. Use the little script python update_tos.py <OrgName>
  5. Log in to the org

Don't forget to change the locale to check if the link is updated, you can add additional locales to update_tos.py, change their name, use en_US or en-us or en or some kind of variations to check if the GUI manages to find the most appropriate version.

# upload_tos.py. It needs requests.
import requests
import sys

response = requests.patch(
    f"http://localhost:6770/administration/organizations/{sys.argv[1]}",
    headers={"Authorization": f"Bearer s3cr3t"},
    json={"tos": {
        "en_US": "https://www.youtube.com/watch?v=dQw4w9WgXcQ",
        "fr_BE": "https://www.youtube.com/watch?v=hEmODTcKJmE",
    }},
)

print(response.status_code, response.content)
  • Keep changes in the pull request as small as possible
  • Ensure the commit history is sanitized
  • Give a meaningful title to your PR
  • Describe your changes
  • Link any related issue in the description

@Max-7 Max-7 marked this pull request as ready for review October 4, 2024 05:43
@Max-7 Max-7 requested review from a team as code owners October 4, 2024 05:43
@FirelightFlagboy

This comment was marked as outdated.

@FirelightFlagboy FirelightFlagboy changed the base branch from master to eula October 4, 2024 06:09
Copy link
Contributor

@FirelightFlagboy FirelightFlagboy left a comment

Choose a reason for hiding this comment

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

I don't see tests for that feature

client/src/locales/en-US.json Outdated Show resolved Hide resolved
@touilleMan touilleMan force-pushed the eula branch 2 times, most recently from e96f74b to fc618f5 Compare October 4, 2024 08:41
Copy link
Contributor

@mmmarcos mmmarcos left a comment

Choose a reason for hiding this comment

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

The "normal" use cases seem to work fine:

  1. ToS defined in server, user logins, ToS dialog is displayed, user accepts ToS, login (OK)
  2. User logout, then login, no ToS dialog is displayed (OK)
  3. User logout, ToS updates in server, user login --> same as 1 (OK)

Issues

Wrong login status

  • User is logged out
  • TOS are updated in the server
  • User tries to login, accepts the new TOS
  • Login status is not updated (KO)
    image
  • Go to home, shows user is logged-in
    image
  • Return to org, login status is still offline (KO)
  • Logout, then login, -- login status is online (OK)

App state if logged-in during ToS update

If the user is logged-in and the TOS are updated on the server. The application behaves unexpectedly:

  • At first, nothing seems to change from the user perspective (menus are accesible, workspaces can be created, folders can be opened, files can be imported, etc.).
  • However, some actions like attempting to sharing a workspace, result in an unrelated error:
    image
    This is very disturbing for the user, who is most certainly not aware of TOS update, and the apps seems to fail wirhout any reason.

What should be done in this case it should be carefully discussed:

  • should the user be forced to log out? not great, since it could be importing files or editing a document...
  • should we ask for TOS acceptance, but let them continue using until next login? asking for acceptance could potentially also block a current user operation
  • should we ignore TOS update until next login? this seems the more natural and user-friendly alternative. Although it may need changes on the current TOS logic.

Copy link
Contributor

@mmmarcos mmmarcos left a comment

Choose a reason for hiding this comment

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

Minor suggestions

client/src/locales/en-US.json Outdated Show resolved Hide resolved
client/src/locales/fr-FR.json Outdated Show resolved Hide resolved
client/src/locales/fr-FR.json Outdated Show resolved Hide resolved
Base automatically changed from eula to master October 4, 2024 13:12
@Max-7 Max-7 mentioned this pull request Oct 7, 2024
6 tasks
@Max-7 Max-7 requested a review from mmmarcos October 8, 2024 07:56
Copy link
Contributor

@mmmarcos mmmarcos left a comment

Choose a reason for hiding this comment

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

Some comments otherwise LGTM (I've mostly reviewed changes to the server/ directory).

newsfragments/7633.feature.rst Outdated Show resolved Hide resolved
server/parsec/components/events.py Outdated Show resolved Hide resolved
client/src/locales/en-US.json Outdated Show resolved Hide resolved
client/src/locales/fr-FR.json Outdated Show resolved Hide resolved
client/src/parsec/terms_of_service.ts Show resolved Hide resolved
client/src/parsec/terms_of_service.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@AureliaDolo AureliaDolo left a comment

Choose a reason for hiding this comment

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

I've looked mostly at the libparsec part and it's looking good.

/// to connect to the server without delay. This is needed since, when offline,
/// the connection monitor has a backoff mechanism to avoid hammering the server
/// with connection attempts.
ShouldRetryConnectionNow,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's an idea for later. but could that event be useful in some other places too ?

Copy link
Contributor

@mmmarcos mmmarcos left a comment

Choose a reason for hiding this comment

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

LGTM (server part)

libparsec/crates/client/src/client/tos.rs Outdated Show resolved Hide resolved
libparsec/crates/client/src/event_bus.rs Outdated Show resolved Hide resolved
@Max-7 Max-7 added this pull request to the merge queue Oct 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 9, 2024
@Max-7 Max-7 enabled auto-merge October 10, 2024 13:08
@Max-7 Max-7 added this pull request to the merge queue Oct 10, 2024
Merged via the queue into master with commit 4ebebfb Oct 10, 2024
15 checks passed
@Max-7 Max-7 deleted the ms-accept-tos branch October 10, 2024 13:31
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.

Implement T&Cs acceptance client-side
6 participants