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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ToS agreement display #6716

Merged
merged 4 commits into from
Oct 30, 2020
Merged

Fix ToS agreement display #6716

merged 4 commits into from
Oct 30, 2020

Conversation

Dynnammo
Copy link
Contributor

@Dynnammo Dynnammo commented Oct 21, 2020 •
edited

馃帺 What? Why?

Currently, the Terms of Service (hereafter named TOS) aren't displayed correctly when a user must accept them.

I simply removed the sticky attributes, therefore the content of TOS would be displayed normally, with the form always displayed below.

It's also more GDPR compliant, as it forces users to read Terms of Services before using the platform

馃搶 Related Issues

馃搵 Subtasks

  • Add CHANGELOG entry
  • Add documentation regarding the feature
  • Add/modify seeds
  • Add tests
  • Another subtask

馃摲 Screenshots (optional)

Before (when signed in and ToS have been modified)
image

After (when signed in and ToS have been modified)
image

馃懟 GIF (optional)

Description

@Dynnammo Dynnammo marked this pull request as ready for review October 26, 2020 08:26
@Dynnammo Dynnammo marked this pull request as draft October 26, 2020 08:26
@Dynnammo Dynnammo marked this pull request as ready for review October 27, 2020 13:20
@Dynnammo
Copy link
Contributor Author

I didn't get why the CI codecov is irregular depending on the commits : for some it reaches 100 %, but the variability of this CI step is weird to me, as I didn't change much files in this PR. I would gladly recheck my work in order to make it compliant in the case I missed something on this subject.

@tramuntanal
Copy link
Contributor

Hi @Dynnammo the ToS are also rendered in the sign up form. Can you please add a snapshot so we can check they also render correctly there?

@tramuntanal tramuntanal self-assigned this Oct 28, 2020
@Dynnammo
Copy link
Contributor Author

Dynnammo commented Oct 28, 2020 •
edited

Hey @tramuntanal . ToS on sign up aren't changed, which is basically struggling : the text is not formated, so no more bold/underlined text etc. ,links and embedded videos. Should it be considered as an issue ? In this case, I seems relevant to me to correct it in a separate PR. What do you suggest ?

EDIT : after verification, this behavior is here since 0.18-stable (at least)

Normal display when not signed / already registered
Normal display

But when accessing sign up page, formatting is gone
Sign up page : formatting is gone...

tramuntanal
tramuntanal suggested changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

hi @Dynnammo
The PR LGTM but I see you've added 3 new i18n files. I18n files should only be managed by crowdin, can you remove them from the PR please?

  • decidim-elections/config/locales/ar-SA.yml
  • decidim-elections/config/locales/cs-CZ.yml
  • decidim-elections/config/locales/sk-SK.yml

tramuntanal
tramuntanal approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Thanks!

@tramuntanal tramuntanal merged commit 84a9231 into decidim:develop Oct 30, 2020
@tramuntanal
Copy link
Contributor

@Dynnammo would you like to backport to `release/0.23-stable'?

@verarojman verarojman mentioned this pull request Nov 20, 2020
Closed
verarojman added a commit to Platoniq/decidim-catencomu that referenced this pull request Nov 20, 2020
@verarojman
Fix terms and conditions bug following decidim/decidim#6716
30c9bfd
@paulinebessoles paulinebessoles mentioned this pull request Nov 24, 2020
Closed
@verarojman verarojman mentioned this pull request Nov 26, 2020
Merged
@tramuntanal tramuntanal mentioned this pull request Dec 3, 2020
Merged
tramuntanal pushed a commit that referenced this pull request Dec 3, 2020
@armandfardeau @Dynnammo
Remove sticky from tos agreement (#6716) (#6954)
1299e15 
* Remove sticky from tos agreement

* Modify tests to check visibility of modified TOS

* Remove i18n files

Co-authored-by: Dynnammo <33259633+Dynnammo@users.noreply.github.com>
@tramuntanal tramuntanal mentioned this pull request Dec 3, 2020
Merged
@mrcasals mrcasals mentioned this pull request Dec 21, 2020
Closed
@mrcasals mrcasals changed the title Remove sticky from tos agreement Fix ToS agreement display Feb 26, 2021
@mrcasals mrcasals added module: core type: fix PRs that implement a fix for a bug labels Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tramuntanal tramuntanal tramuntanal approved these changes

Assignees

@tramuntanal tramuntanal

Labels
in-review module: core type: fix PRs that implement a fix for a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Dynnammo @tramuntanal @mrcasals