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

Refactor index.js to have testable functions #2854

Merged
merged 43 commits into from
Jan 15, 2019
Merged

Refactor index.js to have testable functions #2854

merged 43 commits into from
Jan 15, 2019

Conversation

New0
Copy link
Collaborator

@New0 New0 commented Dec 13, 2018

No description provided.

New0 added 3 commits December 13, 2018 14:04
First export, the onRequest const will be cut into smaller pieces finally have response handlers seperate functions
Cut to minimum peces, then will add the callbacks needed to test return values
@Shelob9 Shelob9 added the PR (Patch) Is a pull request label Dec 18, 2018
@Shelob9
Copy link
Collaborator

Shelob9 commented Dec 18, 2018

Merge #2823 into this branch, so that we can move forward with this, and fix error described here: #2823 (comment) before merging to develop

@Shelob9 Shelob9 added this to the 1.8.0 milestone Dec 18, 2018
package.json Outdated
@@ -53,6 +53,7 @@
"dangerously-set-inner-html": "^2.0.1",
"exports-loader": "^0.7.0",
"inputmask": "^3.3.11",
"jquery": "^3.3.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@New0 Make sure that jQuery is not being bundled (by webapack) should be handled here https://github.com/CalderaWP/Caldera-Forms/blob/master/webpack.config.js#L52)

Also, probably should use 1.12.4 since that's what WordPress uses.

if( response.hasOwnProperty('message') ){
messages[field.fieldIdAttr] = {
error: true,
message: response.hasOwnProperty('message') ? response.message : 'Invalid'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@New0 untranslatable string.

}
};

export const handleFileUploadError = (error, file) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@New0 Inline docs

@@ -1,4 +1,4 @@
const SparkMD5 = require('spark-md5');
export const SparkMD5 = require('spark-md5');

export const getFieldConfigBy = (fieldConfigs, findBy, findWhere) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@New0 Inline docs

@@ -1,4 +1,4 @@
const SparkMD5 = require('spark-md5');
export const SparkMD5 = require('spark-md5');
Copy link
Collaborator

Choose a reason for hiding this comment

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

@New0 Why export the dependency?

@New0
Copy link
Collaborator Author

New0 commented Jan 14, 2019

Ghost inspector test ( test site CF version set to branch 2818-2 ) https://app.ghostinspector.com/tests/5c3882b70b45715fbf380071

@Shelob9
Copy link
Collaborator

Shelob9 commented Jan 15, 2019

@New0 What we said on the call, tests needed in addition to what you have:

  1. 1 field that allows multiple files - add file, add file, remove file, add file, submit, assert all 3 files uploaded
  2. 1 field that allows single file - add file, remove file, add file, submit, assert all 1 file uploaded
  3. 1 field that can be hidden by conditional logic - add file, hide field, submit, assert no files uploaded
  4. 1 field that can be hidden by conditional logic - add file, hide field, unhide submit, assert 1 files uploaded

For the second 2, enzyme could test that the state does not include the file field when field is not hidden -- I think we have this test, not sure.

After that I'll fo final review and MERGE IT 🌋 🌮 🎉 🍰 🌋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR (Patch) Is a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants