Skip to content

Enabling POD providers to enforce a Terms & Conditions - #892

Merged
kjetilk merged 25 commits into
developfrom
feature/link-toc
Nov 6, 2018
Merged

kjetilk merged 25 commits into
developfrom
feature/link-toc

Conversation

@megoth

@megoth megoth commented Oct 31, 2018

Copy link
Copy Markdown
Contributor
  1. Added T&C-steps to options when setting up config
  2. Presenting a checkbox if enforceToc is true
  3. Validating that the checkbox is marked upon creating a new account (if enforceToc is true)

I've also added handlebars-helpers, but right now it's only used one place, so I understand if I should revert it. But I think it can be a useful tool once our templates grow more complex.

Apart from that there are some unnecessary changes, i.e. cleaning up some code and introducing some async/await, that I also can revert if needed.

@ghost ghost assigned megoth Oct 31, 2018
@ghost ghost added the in progress label Oct 31, 2018
@megoth

megoth commented Oct 31, 2018

Copy link
Copy Markdown
Contributor Author

This is a fix for #799

@RubenVerborgh RubenVerborgh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor nits.

Comment thread default-views/account/register-form.hbs Outdated
<div class="checkbox">
<label>
<input type="checkbox" name="acceptToc">
I agree to the <a href="{{tocUri}}" target="_blank">Terms & Conditions</a> of this service

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

HTML encode

Comment thread lib/requests/create-account-request.js Outdated

this.username = options.username
this.userAccount = options.userAccount
this.acceptToc = options.acceptToc === 'true'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should that parsing be this constructor's job?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably fromParams (even though the whole fromParams pattern all over is a disease)

Comment thread lib/requests/create-account-request.js Outdated
}

if (this.enforceToc && !this.acceptToc) {
error = new Error('Accepting Terms & Conditions are required for this service')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

*is

error = new Error('Accepting Terms & Conditions are required for this service')
error.statusCode = 400
throw error
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code duplication?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They are two different classes, so think I'll let it be as is.

But we might want to separate these classes a bit more in the future, i.e. having them in separate files. create-account-request is getting difficult to read.

@megoth

megoth commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

@rubensworks Then I think I've fixed all the nits ^_^ Does it look ok?

@rubensworks

Copy link
Copy Markdown
Contributor

@megoth Wrong Ruben,
I think you meant @RubenVerborgh :-)

@megoth

megoth commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

Hah, sorry about that, did it a little bit fast here =P

@RubenVerborgh does the changes look good?

@ghost ghost added the in progress label Nov 5, 2018
@megoth

megoth commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

I don't have access to security/snyk, so I'm unable to see what stops it

To make it easier for POD-providers to update their own config.json files
@RubenVerborgh

Copy link
Copy Markdown
Contributor

@megoth You can make your own Snyk account to see it.

@RubenVerborgh

Copy link
Copy Markdown
Contributor

Fine with me.

@megoth

megoth commented Nov 6, 2018

Copy link
Copy Markdown
Contributor Author

I decided to remove handlebars-helpers because of a vulnerability it introduced (note to self: check snyk and npm audit when adding new dependencies).

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.

4 participants