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

[16.0][IMP] web_timeline: Handle inverted date ranges #2774

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

houzefa-abba
Copy link
Member

Inverted dates make no functional sense, but when they do happen, this change allows displaying them as single points in the timeline (same as when begin=end).

We currently do have such demo data when installing project_timeline:
Screenshot at 2024-03-21 12-24-31

Before: office design tasks are missing

Screenshot at 2024-03-21 11-53-24

After: office design tasks are shown

Screenshot at 2024-03-21 11-50-51

@OCA-git-bot
Copy link
Contributor

Hi @tarteo,
some modules you are maintaining are being modified, check this out!

@pedrobaeza pedrobaeza added this to the 16.0 milestone Mar 21, 2024
@pedrobaeza
Copy link
Member

I don't get the concept of "inverted date".

@houzefa-abba
Copy link
Member Author

I don't get the concept of "inverted date".

Sorry I wasn't clear enough; this is when end date is before start date (in my example, March 21st ➔ March 19th).

Makes no sense, but at least with this change they won't be completely hidden.

@houzefa-abba houzefa-abba changed the title [16.0][IMP] web_timeline: Handle inverted date ranges WIP: [16.0][IMP] web_timeline: Handle inverted date ranges Mar 21, 2024
@houzefa-abba houzefa-abba marked this pull request as draft March 21, 2024 12:42
@houzefa-abba houzefa-abba changed the title WIP: [16.0][IMP] web_timeline: Handle inverted date ranges [16.0][IMP] web_timeline: Handle inverted date ranges Mar 21, 2024
@houzefa-abba houzefa-abba marked this pull request as ready for review March 21, 2024 12:46
@pedrobaeza
Copy link
Member

I think that should be forbidden by a constraint (SQL or Python).

@houzefa-abba houzefa-abba force-pushed the 16.0-timeline-inverted-dates branch 2 times, most recently from 3999bd5 to 22856c0 Compare April 2, 2024 14:22
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

But the constraint in project_timeline should be added IMO.

@houzefa-abba
Copy link
Member Author

But the constraint in project_timeline should be added IMO.

Yes constraint and demo data, because demo data produces these "reversed dates"

I'll look into it soon

@houzefa-abba
Copy link
Member Author

But the constraint in project_timeline should be added IMO.

Yes constraint and demo data, because demo data produces these "reversed dates"

I'll look into it soon

Done with OCA/project#1259 & OCA/project#1260

Inverted dates make no functional sense, but when they do happen, this
change allows displaying them as single points in the timeline (same as
when begin=end).

We currently do have such demo data when installing project_timeline.
@carlos-lopez-tecnativa
Copy link
Contributor

@houzefa-abba @pedrobaeza Is this PR still needed? The other PRs related to the project have already been merged.

TT50999

@houzefa-abba
Copy link
Member Author

@houzefa-abba @pedrobaeza Is this PR still needed? The other PRs related to the project have already been merged.

TT50999

Hi @carlos-lopez-tecnativa this PR may still help the odd case where someone has data with start-date after end-date

We prevent it in the project part thanks to OCA/project#1260 but web_timeline may be applied in other places and who knows, it may help someone? :)

Copy link
Contributor

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

I agree with @pedrobaeza and believe that it would be more effective to implement constraints that ensure data integrity at the source, preventing such scenarios from occurring in the first place. This is outside the scope of this module and must be controlled on each side. On the other hand, for fixed visual issues, it looks good to me. Thanks @houzefa-abba .

@pedrobaeza
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-2774-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a3f42ba into OCA:16.0 Sep 26, 2024
4 of 5 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 935b007. Thanks a lot for contributing to OCA. ❤️

@houzefa-abba houzefa-abba deleted the 16.0-timeline-inverted-dates branch September 27, 2024 07:50
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Yes this makes sense, thanks. The constaint should be indeed done on model level, this way it shows events where end is before start date.

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

Successfully merging this pull request may close these issues.

5 participants