search

medic / bikram-sambat

Javascript and Java utilities for converting between the Nepali Bikram Sambat (Vikram Samvat) and Gregorian (standard European) calendars.
Apache License 2.0
12 stars 10 forks source link

Fix for issue #17 throwing an error #18

Closed 1yuv closed 1 year ago

1yuv commented 1 year ago

Fix for issue #17:

Currently, a bootstrap sets only a valid BS date and if any invalid date is input, it'll store the previously set date and user won'd know that their input was invalid. This fix will enable bootstrap to pass null if invalid date is inputed by user such that other tools using this component know and handle it correctly.

1yuv commented 1 year ago

Hi @dianabarsan , I've changed the version.

1yuv commented 1 year ago

Hi @jkuester ,

If I enter an invalid value into the widget on a form and save it, will the form just save an empty value for that date?

As you've pointed, If the field is required, it will not accept the form until a valid bikram sambat date is entered and prevent user from saving or moving ahead.

In terms where this field is not required, we'll check if we can add validation like telephone widget. Or do you think returning null when invalid date is given and returning undefined when date is untouched would be a good idea ? I can try this option as well.

1yuv commented 1 year ago

Or do you think returning null when invalid date is given and returning undefined when date is untouched would be a good idea ? I can try this option as well.

This could be really difficult to distinguish it however and we can still try it on CHT-Core by overriding enketo validation and hope that works.

jkuester commented 1 year ago

I am not sure if this would end up having unintended side-effects, but one idea I had was updating the getDate_greg_text function to verify there was a value for the year/month/day before calling toGreg_text. If no values were provided for any of the fields, just return null. If any value was provided, call toGreg_text, but don't do it in the try-block. If the values are invalid, toGreg_text will throw the error and maybe it can bubble back up to the widget where it can be caught and interpreted as an invalid date. We would need to do some more investigation around this, though, to make sure it actually would work and would not cause unintended consequences (in the widget or other places where the library is used...).

Ultimately, I am happy to follow your discretion on this issue. If you feel that the current change is sufficient and it is a lower priority to alert the user that they entered an invalid date that will not be saved I am absolutely fine with moving ahead with things like they are now in the PR. We can log a new issue with our thoughts on this extra validation.

jkuester commented 1 year ago

Thinking about this even more, I realized that my proposed code chance was in the bikram-sambat-bootstrap file and not in the underlying index.js for the bikram-sambat package. AFAICT this bikram-sambat-bootstrap is only ever used by the bikram-sambat-datepicker, so as long as it works for our needs there, I don't think we have to worry much about unintended consequences in other parts of the code....