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

chore: remove form_field.isFutureOnly key #235

Merged
merged 5 commits into from
Sep 2, 2020
Merged

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Aug 31, 2020

Problem

Closes #40

Tests

Create 5 date fields in admin view with the following options. Test that the validations work in the preview panel datepicker on the right (i.e. the popup calendar).

  • Disallow past dates
  • Disallow future dates
  • Custom date range with Min Date only
  • Custom date range with Max Date only
  • Custom date range with both Min and Max Date

Validation for date range validation option in admin view

  • Check that it is not possible for admin to save a form with a custom date range where both Min and Max date are left blank
  • Check that it is not possible for admin to save a form with a custom date range where Max date is before Min date

Able to submit forms correctly

  • Publish the form. Test that all 5 date field validations still work in the public form datepicker (i.e. the popup calendar)
  • Test that all 5 date field validations still work in the public form using manual input to the date field and manual input of date outside the validation range is rejected

Test for editing and reordering, instead of creating

  • Using the existing form fields, shuffle the form validations of the five fields by editing the validation options
  • Reorder the form fields
  • Repeat the above tests to make sure the date validations remain correct

Scripts

  • DB script in remove-isfutureonly-key.js, to be run one week after release

@tshuli tshuli requested a review from karrui August 31, 2020 11:49
@tshuli tshuli marked this pull request as ready for review August 31, 2020 11:49
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

could you also include a db script to remove the old key? as recent experience has shown, we should avoid leaving old keys dangling

@karrui
Copy link
Contributor

karrui commented Sep 2, 2020

Have you been committing with --no-verify? That defeats the point of hooks :/.

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

Some changes, I'll leave it to @mantariksh to really review the script ><

scripts/date-logic/date-logic.js Outdated Show resolved Hide resolved
scripts/date-logic/date-logic.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

lgtm

scripts/date-logic/remove-isfutureonly-key.js Outdated Show resolved Hide resolved
scripts/date-logic/remove-isfutureonly-key.js Outdated Show resolved Hide resolved
@karrui
Copy link
Contributor

karrui commented Sep 2, 2020

Sorry for the many separate comments, basically just try again on localhost and see if any errors pop up

@tshuli tshuli requested a review from karrui September 2, 2020 08:12
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm

@tshuli tshuli merged commit a8d9962 into develop Sep 2, 2020
@liangyuanruo liangyuanruo deleted the remove-isfutureonly-key branch September 2, 2020 09:04
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.

chore: remove form_field.isFutureOnly key
3 participants