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

feat: stealth textfield prefill feature from query parameters #524

Merged
merged 15 commits into from
Oct 28, 2020
Merged
3 changes: 1 addition & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@
"font-awesome": "4.7.0",
"fp-ts": "^2.8.3",
"has-ansi": "^4.0.0",
"he": "^1.2.0",
"helmet": "^4.1.1",
"http-status-codes": "^2.1.4",
"intl-tel-input": "~12.1.6",
Expand Down
8 changes: 7 additions & 1 deletion src/app/controllers/public-forms.server.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const mongoose = require('mongoose')
const { StatusCodes } = require('http-status-codes')
const querystring = require('querystring')

const { createReqMeta } = require('../utils/request')
const logger = require('../../config/logger').createLoggerWithLabel(module)
Expand Down Expand Up @@ -47,6 +48,10 @@ exports.redirect = async function (req, res) {
let redirectPath = req.params.state
? req.params.Id + '/' + req.params.state
: req.params.Id
const reqQuery = querystring.stringify(req.query)
if (reqQuery.length > 0) {
redirectPath = redirectPath + '?' + encodeURIComponent(reqQuery)
}
try {
const metaTags = await fetchMetatags(req)
return res.render('index', {
karrui marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -62,8 +67,9 @@ exports.redirect = async function (req, res) {
},
error: err,
})
res.redirect('/#!/' + redirectPath)
return
Comment on lines +70 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
res.redirect('/#!/' + redirectPath)
return
return res.redirect('/#!/' + redirectPath)

}
res.redirect('/#!/' + redirectPath)
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/app/routes/frontend.server.routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module.exports = function (app) {
celebrate({
[Segments.QUERY]: {
redirectPath: Joi.string()
.regex(/^[a-fA-F0-9]{24}(\/(preview|template|use-template))?$/)
.regex(/^[a-fA-F0-9]{24}(\/(preview|template|use-template))?/)
.required(),
},
}),
Expand Down
28 changes: 26 additions & 2 deletions src/public/modules/forms/base/directives/field.client.directive.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
'use strict'

const { get } = require('lodash')
const he = require('he')
const querystring = require('querystring')

angular
.module('forms')
.directive('fieldDirective', ['FormFields', fieldDirective])
.directive('fieldDirective', ['FormFields', '$location', fieldDirective])

function fieldDirective(FormFields) {
function fieldDirective(FormFields, $location) {
return {
restrict: 'E',
templateUrl:
Expand All @@ -23,6 +25,28 @@ function fieldDirective(FormFields) {
isValidateDate: '<',
},
link: function (scope) {
// Stealth prefill feature
// If a query parameter is provided to a form URL in the form ?<fieldId1>=<value1>&<fieldId2>=<value2>...
// And if the fieldIds are valid mongoose object IDs and refer to a short text field,
// Then prefill and disable editing the corresponding form field on the frontend

const decodedUrl = he.decode($location.url()) // tech debt; after redirect, & is encoded as &amp; in the query string
const query = decodedUrl.split('?')[1]
Copy link
Contributor

@liangyuanruo liangyuanruo Oct 28, 2020

Choose a reason for hiding this comment

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

can i confirm there's no null pointer exception if the URL doesn't contain ?? i.e. the standard use case

Copy link
Contributor

Choose a reason for hiding this comment

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

or index out of bounds with the [1] index access

Copy link
Contributor Author

@tshuli tshuli Oct 28, 2020

Choose a reason for hiding this comment

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

yes, there is no null pointer exception because the url string includes the formid i.e. it is not null

accessing index out of bounds returns undefined, but since this isn't in the specifications i've added an array length check

const queryParams = querystring.parse(query)

if (
scope.field._id in queryParams &&
scope.field.fieldType === 'textfield'
) {
const prefillValue = queryParams[scope.field._id]
if (typeof prefillValue === 'string') {
// Only support unique query params. If query params are duplicated,
// none of the duplicated keys will be prefilled
scope.field.fieldValue = prefillValue // do not use $sanitize as it removes non-english characters, tested that script and html tags do not work
liangyuanruo marked this conversation as resolved.
Show resolved Hide resolved
scope.field.disabled = true
}
}

if ((scope.isadminpreview || scope.isTemplate) && scope.field.myInfo) {
// Determine whether to disable field in preview
if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ describe('Public-Forms Controller', () => {
params: {},
body: {},
headers: {},
url: '',
ip: '127.0.0.1',
get: () => '',
}
Expand Down Expand Up @@ -90,6 +91,35 @@ describe('Public-Forms Controller', () => {
Controller.redirect(req, res)
})

it('should redirect to form with correct query params retained', (done) => {
req.params = {
Id: '321564654f65we4f65e4f5',
}
req.query = {
p1: 'v1',
p2: 'v2',
p3: ['v3', 'v4'],
}

res.redirect = jasmine.createSpy().and.callFake(() => {
expect(res.redirect).toHaveBeenCalledWith(
jasmine.stringMatching(/\?.*p1%3Dv1/),
)
expect(res.redirect).toHaveBeenCalledWith(
jasmine.stringMatching(/\?.*p2%3Dv2/),
)
expect(res.redirect).toHaveBeenCalledWith(
jasmine.stringMatching(/\?.*p3%3Dv3/),
)
expect(res.redirect).toHaveBeenCalledWith(
jasmine.stringMatching(/\?.*p3%3Dv4/),
)
done()
})

Controller.redirect(req, res)
})
liangyuanruo marked this conversation as resolved.
Show resolved Hide resolved

it('should render index if getting fetchMetatags succeeds', (done) => {
req.params = {
Id: testForm._id,
Expand Down