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

GDPR terms popup consent #180

Merged
merged 1 commit into from
May 6, 2019
Merged

GDPR terms popup consent #180

merged 1 commit into from
May 6, 2019

Conversation

Lullebullelukas
Copy link
Contributor

Added function that prompts user to accept terms of use, also made it so that terms version and api is fetched from server at app start. Changed version_check to use the api that is fetched.

$.getJSON(API + '/versions')
.done(function(resp) {
if (resp.current_version !== appAPIVersion) {
function checkAPIVersion() {

Choose a reason for hiding this comment

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

'checkAPIVersion' is defined but never used no-unused-vars

www/js/index.js Outdated
checkAPIVersion();
})
.fail(function(resp) {
console.log(resp.statusText);

Choose a reason for hiding this comment

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

Unexpected console statement no-console

@@ -42,6 +42,10 @@ const AC_URL = 'wss://stage.fsektionen.se/cable';
// ActionCable Token URL
const AC_TOKEN_URL = API + '/messages/new_token';

// GDPR terms version
var termsVersion;
var apiVersion;

Choose a reason for hiding this comment

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

'apiVersion' is assigned a value but never used no-unused-vars

@@ -42,6 +42,10 @@ const AC_URL = 'wss://stage.fsektionen.se/cable';
// ActionCable Token URL
const AC_TOKEN_URL = API + '/messages/new_token';

// GDPR terms version
var termsVersion;

Choose a reason for hiding this comment

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

'termsVersion' is assigned a value but never used no-unused-vars

$.auth.user.terms_version = termsVersion;
})
.fail(function() {
console.log("Terms_version ändrades inte")

Choose a reason for hiding this comment

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

Missing semicolon semi
Strings must use singlequote quotes
Unexpected console statement no-console

@@ -0,0 +1,30 @@
function checkTermsVersion (termsVersion) {

Choose a reason for hiding this comment

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

'checkTermsVersion' is defined but never used no-unused-vars

Copy link
Member

@FredrikLastow FredrikLastow left a comment

Choose a reason for hiding this comment

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

Veri nice my man! Have a few comments that you should fix though:

  1. Combine both version checks (checkAPIVersion() and checkTermsVersion()) in the same js file which could be called version_checks.js
  2. Change the template name for the api to APIVersionTemplate

www/index.html Outdated
<div class="popup-text">För att fortsätta använda f-appen och vår hemsida måste du acceptera att vi använder din användardata.
<a class="item-link external" target="_blank"
href="https://fsektionen.se/sekretess">läs mer här</a>
<p><a class="link button button-fill GDPRaccept popup-close" href="#">Jag accepterar</a></p>
Copy link
Member

Choose a reason for hiding this comment

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

Remove the <p> tag and change the class name. See comment below for the name change.

Suggested change
<p><a class="link button button-fill GDPRaccept popup-close" href="#">Jag accepterar</a></p>
<a class="link button button-fill accept-terms popup-close" href="#">Jag accepterar</a>

});
popup.open();
//Changes user data to make sure they only have to accept GDPR once
$('.GDPRaccept').on('click', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Try to be consistent with names and stick to terms rather than GDRP. Something like this would be better:

Suggested change
$('.GDPRaccept').on('click', function () {
$('.accept-terms').on('click', function () {

})
.done(function() {
$.auth.user.terms_version = termsVersion;
});
Copy link
Member

Choose a reason for hiding this comment

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

Indent properly. .done should have the same tabbing as $.ajax

www/index.html Outdated
</body>
<script type="text/javascript" src="js/adventures.js"></script>
<script type="text/javascript" src="js/nollning.js"></script>-->
</body>
Copy link
Member

Choose a reason for hiding this comment

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

Remove this indent.

Suggested change
</body>
</body>

www/index.html Outdated
<div class="popup popup-terms">
<div class="background-overlay"></div>
<div class="block">
<div class="popup-text">För att fortsätta använda f-appen och vår hemsida måste du acceptera att vi använder din användardata.
Copy link
Member

Choose a reason for hiding this comment

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

Change the text to match the website

Suggested change
<div class="popup-text">För att fortsätta använda f-appen och vår hemsida måste du acceptera att vi använder din användardata.
<div class="popup-text">Du behöver läsa igenom och godkänna den senaste versionen av F-sektionens sekretessvillkor innan du kan fortsätta. Sekretessvillkoren finns att läsa

.done(function(resp) {
if (resp.current_version !== appAPIVersion) {
function checkAPIVersion() {
const appAPIVersion = 1.1;
Copy link
Member

Choose a reason for hiding this comment

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

Thinks this name makes more sense.

Suggested change
const appAPIVersion = 1.1;
const localAPIVersion = 1.1;

@@ -0,0 +1,26 @@
function checkTermsVersion (termsVersion) {

var templateHTML = app.templates.checkTermsVersionTemplate({});
Copy link
Member

Choose a reason for hiding this comment

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

Think this name makes more sense.

Suggested change
var templateHTML = app.templates.checkTermsVersionTemplate({});
var templateHTML = app.templates.termsVersionTemplate({});

www/index.html Outdated
@@ -977,7 +977,22 @@
{{/each image}}
</div>
</script>
</section>

<script type="text/template7" id="checkTermsVersionTemplate">
Copy link
Member

Choose a reason for hiding this comment

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

Think this name makes more sense.

Suggested change
<script type="text/template7" id="checkTermsVersionTemplate">
<script type="text/template7" id="termsVersionTemplate">

www/js/index.js Outdated
document.addEventListener('deviceready', function() {
document.addEventListener('backbutton', onBackKey, false);
$.getJSON(API + '/versions')
.done(function(resp) {

Copy link
Member

Choose a reason for hiding this comment

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

Remove empty line.

})
.fail(function(resp) {
console.log(resp.statusText);
var templateHTML = app.templates.versionCheckTemplate({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var templateHTML = app.templates.versionCheckTemplate({
var templateHTML = app.templates.APIVersionTemplate({

www/js/login.js Outdated
//Checks if user has accepted the latest terms and prompts them if they have not
checkTermsVersion(termsVersion);


Copy link
Member

Choose a reason for hiding this comment

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

Remove empty line here as well.

terms_version: termsVersion
}}
})
.done(function() {

Choose a reason for hiding this comment

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

Expected indentation of 8 spaces but found 6 indent

* they have not.
*/

function checkTermsVersion (termsVersion) {

Choose a reason for hiding this comment

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

'checkTermsVersion' is defined but never used no-unused-vars

appLink = androidAppLink;
}

var templateHTML = app.templates.APIVersionTemplate({

Choose a reason for hiding this comment

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

A function with a name starting with an uppercase letter should only be used as a constructor new-cap

appLink: appLink,
easter: false
});
function checkAPIVersion() {

Choose a reason for hiding this comment

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

'checkAPIVersion' is defined but never used no-unused-vars

}

.button.accept-terms {
margin-top: 45px;

Choose a reason for hiding this comment

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

Properties should be ordered background-color, color, font-size, height, margin-top, padding-top

font-weight: bold;
}

a.item-link {

Choose a reason for hiding this comment

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

Avoid qualifying class selectors with an element.

@@ -20,6 +20,29 @@
color: white;
}

.popup-terms {
.block-title {
font-size: 22px;

Choose a reason for hiding this comment

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

Properties should be ordered color, font-size, font-weight, margin-top

www/js/version_check.js Show resolved Hide resolved
www/js/version_check.js Show resolved Hide resolved
function checkTermsVersion (termsVersion) {

var templateHTML = app.templates.termsVersionTemplate({});
console.log($.auth.user.terms_version);

Choose a reason for hiding this comment

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

Unexpected console statement no-console

console.log(termsVersion);
apiVersion = resp.api_version;
console.log(apiVersion);
console.log(resp);

Choose a reason for hiding this comment

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

Unexpected console statement no-console

termsVersion = resp.terms_version;
console.log(termsVersion);
apiVersion = resp.api_version;
console.log(apiVersion);

Choose a reason for hiding this comment

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

Unexpected console statement no-console

$.getJSON(API + '/versions')
.done(function(resp) {
termsVersion = resp.terms_version;
console.log(termsVersion);

Choose a reason for hiding this comment

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

Unexpected console statement no-console

Copy link
Member

@FredrikLastow FredrikLastow left a comment

Choose a reason for hiding this comment

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

U DA MAN, MAN! +1

@Lullebullelukas Lullebullelukas merged commit f55d851 into master May 6, 2019
@Lullebullelukas Lullebullelukas deleted the lukas-GDPRpopupyey branch May 6, 2019 17:15
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.

3 participants