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

Fixes issue #3001. datepicker disables dates before 1st Jan #3008

Open
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@terencemo
Contributor

terencemo commented Sep 1, 2018

Description

For the datatable date input field, min value was set to minDate. Issue was it was taking some default min value which was 1st of Jan of the current year so when the min value was explicitly set, it allows earlier dates.

Related issues and discussion

#3001

Screenshots, if any

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Validate the JS and HTML files with grunt validate to detect errors and potential problems in JavaScript code.

  • If you have multiple commits please combine them into one commit by squashing them.

  • Read and understood the contribution guidelines at community-app/Contributing.md.

@ShruthiRajaram

Make similar changes for DateTime field also

@terencemo

This comment has been minimized.

Contributor

terencemo commented Sep 4, 2018

Thanks for your suggestion. Have you tested and confirmed that the similar issue exists with DateTime field? If so, suggest that we track DateTime as a separate issue.

Rationale: Good software development practice is to create separate tickets for separate issues:
Mozilla Guide says separate tickets for separate issues:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines

Another guide says be specific:
https://www.softwaretestinghelp.com/how-to-write-good-bug-report/
To quote:

Do not combine multiple problems even if they seem to be similar. Write different reports for each problem.

As an Open Source project, I think if there is no technical issues with PR and it fixes the issue, it should be accepted. Even if contributors had the bandwidth to add multiple fixes into single tickets, it is better to track issues separately.

@ShruthiRajaram

This comment has been minimized.

Collaborator

ShruthiRajaram commented Sep 5, 2018

@terencemo thanks for sharing guidelines. Yes, the date time issue is easily identifiable and should have been covered in the issue.
I suggested additional fix with existing change, since both will affect date field and if user wants to have a date time field instead of date field, the PR will not serve solution completely. At that time you will end up creating one more issue and a separate PR for the same date field. Technically this should have been covered in the issue, but wasn't.
Its up-to you how you want to proceed.

@terencemo

This comment has been minimized.

Contributor

terencemo commented Sep 5, 2018

@ShruthiRajaram currently I don't have bandwidth to test the DateTime field and I think it should be a separate issue. There are organisations using Mifos X which have DataTables with only date fields. This will solve the issue they are facing. I haven't received a clear answer whether you have tested and can confirm the issue occurs for DateTime fields. If you have, suggest you to raise a separate issue. Also, you say

Technically this should have been covered in the issue, but wasn't.

Can you explain the basis of this assessment?

@edcable, @mgeiss, @nazeer1100126 if your team has consensus that you don't want to create separate tickets to track different issues and wan't to combine related tickets, would you suggest we close this PR? Also it will be helpful for contributors to update https://github.com/openMF/community-app/blob/develop/Contributing.md regarding combining issues in a PR. If we are deviating from mainstream Open Source practices, I think we should document the same properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment