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 pass params into forms #2674

Merged
merged 5 commits into from Feb 24, 2023

Conversation

xmorave2
Copy link
Contributor

This enables to create links with urls like /Feedback/Form/SomeForm?param=value to prefill inputs for user, or fill hidden inputs with some dynamic value.

@demiankatz
Copy link
Member

Thanks, @xmorave2! I'm curious whether @EreMaijala has any thoughts on this, since he's more invested in the form code than I am. I also wonder whether we need to add test coverage for this new functionality, and whether we should add a configuration setting to limit which parameters may be pre-filled via URL (in case there are any possible abuses of the functionality that we might want to avoid). Any thoughts?

Copy link
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

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

This is a good idea, but @demiankatz has valid points. We need to protect at least the read-only reserved elements (referrer, useragent, submit). Also need to ensure that this doesn't allow overriding a select field with an invalid value (I didn't test it yet), and here test coverage would help.

@xmorave2 xmorave2 self-assigned this Feb 13, 2023
@xmorave2
Copy link
Contributor Author

Thanks for review.

I added configuration for enabling prefill only for specific fields and check if some of protected fields are not prefilled.

I didn't implement prefilling for selects and checkboxes - it turned out a bit complicated and we don't even need for our use case.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for the progress on this! I have just a few minor suggestions.

config/vufind/FeedbackForms.yaml Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Form/Form.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Form/Form.php Outdated Show resolved Hide resolved
@xmorave2
Copy link
Contributor Author

Thanks @demiankatz I fixed them all.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @xmorave2! I'll wait to see if @EreMaijala has any further thoughts before merging this.

Copy link
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

@demiankatz demiankatz merged commit a4241e2 into vufind-org:dev Feb 24, 2023
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants