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

fix: issue/2090 mapStateToCellProps should use translated error messages #2098

Merged
merged 4 commits into from
Mar 7, 2023

Conversation

codefactor
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2023

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Feb 20, 2023

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit e9d7406
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/640752e336e4f8000848bd84
😎 Deploy Preview https://deploy-preview-2098--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sdirix
Copy link
Member

sdirix commented Feb 20, 2023

Thanks for the contribution ❤️

Please make sure to sign the CLA in a way which is recognizable by the CLA bot.

@codefactor
Copy link
Contributor Author

@sdirix ,
I've now updated the CLA so the bot should be satisfied.

@codefactor
Copy link
Contributor Author

codefactor commented Feb 22, 2023

@sdirix ,
Try as I might I could not figure out the unit test so I just rolled the unit test back. I believe the tests will pass without covering the case where the mapStateToCellProps function is given a state which contains an error object array. The commit will be in the history of the cell.test.ts I believe, so it would be possible to revert the revert in the future if adding unit test coverage to those lines is important.

Unfortunately, I can't quite figure out why there is a test/util/cell.test.ts exited due to SIGABRT error happening. After some more debugging there does appear to be some minor issues regarding the setup and the assertions - you can see the unit test I removed here:

9828188#diff-7d5366084c8446977a6750222516b31ab17a6b20eda83d02a9ec9848f28cd172L277-L298

I'm pretty sure the ownProps is missing the schema and the path props which is causing the errorAt functions to filter out all errors, also the errors array is in the wrong part of the state, I believe it should be in state.jsonforms.core.errors and not state.jsonforms.errors. But even if I correct all of those issues, the tests still crash. Under normal circumstances, this should cause the mapStateToCellProps function to return an empty string for the errors prop and then fail the test with an assertion error; however, the whole test just bombs.

Please let me know how you think we should proceed on this one.

@sdirix
Copy link
Member

sdirix commented Feb 22, 2023

@codefactor Thanks for the work and analysis. We'll take a look 👍

@coveralls
Copy link

coveralls commented Feb 22, 2023

Coverage Status

Coverage: 83.193%. Remained the same when pulling e9d7406 on codefactor:defect/2090 into 3861749 on eclipsesource:master.

@codefactor
Copy link
Contributor Author

@sdirix what are the next steps on this PR? Do you need more test coverage to get it merged?

@sdirix
Copy link
Member

sdirix commented Mar 7, 2023

Depending on our work load it can take some time until we are able to look at topics like this in detail. The good news is I was able to fix the test ;)

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Works for me!

@sdirix sdirix merged commit 2bbc646 into eclipsesource:master Mar 7, 2023
@sdirix sdirix linked an issue Mar 7, 2023 that may be closed by this pull request
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.

mapStateToCellProps does not translate error messages
4 participants