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

Foorm: Add published state to all forms and add to seed step #37655

Merged
merged 6 commits into from Nov 5, 2020

Conversation

molly-moen
Copy link
Contributor

We added a published column for foorm forms and library questions in this PR. This PR adds a published state for every form and library question, and seeds that data in as part of the seed step. This state will be used to help provide better information to our survey editors. I also updated the API that our editor uses to get the published state, but am not using it in the editor yet. That will be done in a follow-up PR.

Future work

As a follow-up I will add this published state to the editor UI in a more friendly way (currently it is visible in the editor as part of the json). When we add the ability to save forms from the editor we will also want to add the ability to change a survey from draft->published mode without the survey author knowing the json syntax (i.e., a button that says "publish" that will inject the correct json field to the survey configuration). There is also a P1 item to make this published state more than just an FYI for survey authors--we will block viewing of draft surveys on prod, and only allow certain updates to already-published surveys.

Links

Testing story

Tested locally that the forms and library questions seed correctly, and the new published state does not mess up any surveys. Surveyjs ignores all fields it does not know about so there are no issues there.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@@ -18,6 +18,7 @@ const styles = {
class FoormEditorManager extends React.Component {
static propTypes = {
updateFormQuestions: PropTypes.func,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the updateFormQuestions prop be removed? Or is it used elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh it looks like it's still being passed as a prop in apps/src/sites/studio/pages/foorm/editor/index.js -- maybe just help me understand why it's sticking around and most other places language like FormQuestions is being swapped out for FormData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question. The form data is an object in the form {published: true/false, questions: {entire json blob of survey}}, while form questions is the json blob of the survey. We get the form data once when loading a survey, then send around the form questions as the survey author edits them. I can add a comment somewhere to make this clearer.

@@ -43,12 +43,15 @@ def self.setup

# Let's load the JSON text.
questions = File.read(path)
# if published is not provided, default to true
published = questions['published'].nil? ? true : questions['published']
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Shouldn't the default you set on the published column coerce it to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's necessary. If we leave the value as nil it will treat the value as nil, not change it to the default. The default is only used if no value is provided, so we need to check that the value is provided somewhere.

Copy link
Contributor

@bencodeorg bencodeorg left a comment

Choose a reason for hiding this comment

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

Admittedly still not super comfortable with Redux, but looks good as far as I can tell! Would be nice to add a comment to distinguish between updateFormQuestions and updateFormData props.

@molly-moen molly-moen merged commit 3c676c2 into staging Nov 5, 2020
@molly-moen molly-moen deleted the foorm-published-state branch November 5, 2020 17:24
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.

None yet

2 participants