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 date issues when user inputs: date out of bound or date_to earlier than date_from #33

Merged
merged 15 commits into from
Feb 9, 2024

Conversation

toropok
Copy link
Contributor

@toropok toropok commented Jan 4, 2024

Description of the issue/feature this PR addresses

fix for #32 issue

@toropok
Copy link
Contributor Author

toropok commented Jan 4, 2024

I dont remember should I add that to changes.rst in the core project :)

@ramonski
Copy link
Contributor

ramonski commented Jan 4, 2024

I dont remember should I add that to changes.rst in the core project :)

Hi Leonid,
Thanks for the fixture!

Please always update the Changelog in the Add-on where the changes happened:
https://github.com/senaite/senaite.databox/blob/master/docs/Changelog.rst

Thanks

@@ -57,6 +57,7 @@
class DataBoxView(ListingView):
"""The default DataBox view
"""
from senaite.core.api import dtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it have a reason why you added the import at class level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recall the reason was that dtime was unreachable from a template expression statement, if import statement declared for module level..

Would you think its better to return import statement to the module level and provide sort of wrapper from DataBoxView class to that template expression? Or even somehow import dtime right in template (is that possible?)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that's strange... Maybe this is because of restricted Python, but not sure.
Anyhow, I think that accessing the module directly is bad style and should be avoided.
Better create two more property methods in the class, that return the right value for you, e.g.:

@property
def date_from(self):
    if not self.context.date_from:
        return ""
    return dtime.date_to_string(self.context.date_from)

And use then this in your template:

<input type="date"
       class="form-control"
       tal:attributes="value python:view.date_from"
       name="senaite.databox.date_from"/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I've added properties.

Also, I've added date_to > date_from comparison, which should make report behaviour more intuitive. In case user puts date_to earlier than date_from - we use [date_from, date_from] range for date query.

@toropok toropok changed the title Fix prevent strftime() error raised if user enters out of range date Fix date date issues when user inputs: date out of bound or date_to earlier then date_from Jan 8, 2024
@toropok toropok changed the title Fix date date issues when user inputs: date out of bound or date_to earlier then date_from Fix date issues when user inputs: date out of bound or date_to earlier then date_from Jan 8, 2024
@toropok toropok changed the title Fix date issues when user inputs: date out of bound or date_to earlier then date_from Fix date issues when user inputs: date out of bound or date_to earlier than date_from Jan 8, 2024
Copy link
Contributor

@ramonski ramonski left a comment

Choose a reason for hiding this comment

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

Hi @toropok,
apologies for late merging! I no longer had the PR on my radar ...

@ramonski ramonski merged commit 142cd8f into senaite:master Feb 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants