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

Add ability to update T+Cs to cause a re-review / reaccept for users #951

Merged
merged 19 commits into from
Sep 14, 2022

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Sep 7, 2022

fixes #451

TODO:

  • update design on story
  • tests

@Steve-Mcl Steve-Mcl marked this pull request as ready for review September 8, 2022 10:19
@Steve-Mcl
Copy link
Contributor Author

@knolleary this is now ready for review.

NOTE: e2e test failing on team membership as your licence limits PR

  FlowForge - Team Membership
    ✓ loads team members into the data table (3777ms)
    ✓ member cannot can invite or remove a user (3108ms)
    ✓ owner/admin can invite user (3578ms)
    ✓ loads invitations into the data table (2318ms)
    1) user can accept a team invite
    2) admin/owner can remove a member from the team


  4 passing (27s)
  2 failing

  1) FlowForge - Team Membership
       user can accept a team invite:
     AssertionError: Timed out retrying after 4000ms: Too many elements found. Found '1', expected '0'.
      at Context.eval (http://localhost:3000/__cypress/tests?p=test/e2e/frontend/cypress/tests/team-membership.spec.js:142:60)

  2) FlowForge - Team Membership
       admin/owner can remove a member from the team:

      AssertionError: Timed out retrying after 4000ms: Not enough elements found. Found '3', expected '4'.
      + expected - actual

      -3
      +4

... this should be fixed when 9921b43 is merged

@knolleary
Copy link
Member

We already have a migration in main that has the prefix forge/db/migrations/20220905-01-. It's complete filename comes after the one added by this PR, which will mean this migration will be skipped in any local install that has already run the main code.

Can you rename the migration file to today's date?

@knolleary
Copy link
Member

Sorry - I mucked up resolving the merge conflict. Have pushed the fix.

forge/db/views/User.js Outdated Show resolved Hide resolved
frontend/src/pages/admin/Settings/General.vue Outdated Show resolved Hide resolved
@knolleary
Copy link
Member

Generally looks good - but I think some UX changes are needed on the admin settings page.

I have worked up the following. LMK what you think:

image

Note I have proposed here that changing the T&Cs url should also trigger an update. How does that sound? Or do we want the admin to be able to change the url without triggering re-acceptance?

@Steve-Mcl
Copy link
Contributor Author

Generally looks good - but I think some UX changes are needed on the admin settings page.

I have worked up the following. LMK what you think:

image

Note I have proposed here that changing the T&Cs url should also trigger an update. How does that sound? Or do we want the admin to be able to change the url without triggering re-acceptance?

Very much agree - and i did try (and failed) so left it as was

If you have the necessary html to how you achieved the cleaner look - please send it my way (or update the PR)

Either way let me know.

Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
@Steve-Mcl
Copy link
Contributor Author

We already have a migration in main that has the prefix forge/db/migrations/20220905-01-. It's complete filename comes after the one added by this PR, which will mean this migration will be skipped in any local install that has already run the main code.

Can you rename the migration file to today's date?

What are the odds we both wrote DB migrations on the same date lol

Will push update shortly

@knolleary
Copy link
Member

I have pushed the UI changes.

One more piece to add (updating the date when the url is changed)... will update here when done

@knolleary
Copy link
Member

I've updated the logic around saving admin settings to ensure tcs-updated gets set at the right times.

There was also an undiscovered bug around detecting if the tcs url has changed in the saveEnabled computed property.

@Steve-Mcl
Copy link
Contributor Author

Tests updated to ensure presence of/absence of tcs settings in the user API

Added a couple more tests that seem appropriate...

  • Terms and Conditions
    • admin can enable Terms and Conditions
    • admin can update Terms and Conditions date
    • user can accept Terms and Conditions
    • user API does not return T&Cs properties if T&Cs are disabled

fix changes in sequence caused by `tcs-date` update when url is changed
@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Sep 13, 2022

@knolleary - finally passing e2e tests once more.

The modification you pushed that sets the T+Cs date upon URL change caused the e2e tests to get stuck expecting something other than the T+Cs dialog.

Sorted now.

@knolleary knolleary merged commit 40ff9da into main Sep 14, 2022
@knolleary knolleary deleted the add-tcs-update branch September 14, 2022 08: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.

Be able to update T&Cs and prompt users to accept update
2 participants