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

Explicitly identify the value that can't be converted to a date #494

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

lognaturel
Copy link
Member

In response to #405

What has been done to verify that this works as intended?

Added tests to make sure the exceptions are thrown when expected.

Why is this the best possible solution? Were any other approaches considered?

In Collect, these exception messages will be shown to the user in a dialog if re-computation leads to an invalid value being passed to a function that converts to date. These error messages should help the user identify what form design improvements they could make to avoid that case without making them too long. I considered providing suggestions like adding constraints or defaults but I think it's too hard to make generically useful.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

All this should do is improve error text.

Do we need any specific form for testing your changes? If so, please attach one.

decimal-date-time-bug.xls.zip

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

@codecov-io
Copy link

codecov-io commented Sep 26, 2019

Codecov Report

Merging #494 into master will increase coverage by 0.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #494      +/-   ##
============================================
+ Coverage     50.01%   50.02%   +0.01%     
- Complexity     3016     3017       +1     
============================================
  Files           247      247              
  Lines         13797    13797              
  Branches       2674     2674              
============================================
+ Hits           6900     6902       +2     
+ Misses         6051     6050       -1     
+ Partials        846      845       -1
Impacted Files Coverage Δ Complexity Δ
...in/java/org/javarosa/xpath/expr/XPathFuncExpr.java 69.96% <57.14%> (+0.3%) 208 <0> (+1) ⬆️
...org/javarosa/core/model/condition/Triggerable.java 67.76% <0%> (-1.66%) 25% <0%> (ø)
.../java/org/javarosa/core/model/utils/DateUtils.java 58.33% <0%> (+0.57%) 72% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84c3bdf...798bdeb. Read the comment docs.

Copy link
Contributor

@ggalmazor ggalmazor left a comment

Choose a reason for hiding this comment

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

LGTM.

I understand that this PR doesn't try to solve the core issue reported in #405.

@lognaturel lognaturel merged commit 329514c into getodk:master Oct 1, 2019
@lognaturel lognaturel deleted the issue-405 branch October 1, 2019 19:53
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.

3 participants