Bug in expiration date for end of month - it updates to previous month

[dlpzx]

Describe the bug

When running the integration tests (current ones) the tests/modules/s3_datasets_shares/test_share.py::test_submit_share_extension_request_with_auto_approval test fails: https://github.com/data-dot-all/dataall/actions/runs/11102824595/job/30843468276?pr=1573

The issue seems to be in the expiration date set to test the share expiration dates on share submission.

Checking the logs of getShareObject after the request is submitted:
'expiryDate': '2024-09-30 00:00:00', 'requestedExpiryDate': '2024-09-30 00:00:00', ---> which means that share.requestedExpiryDate < datetime.today() is True so the test falls into this block from the share_object_service:

            if dataset.enableExpiration and share.requestedExpiryDate and share.requestedExpiryDate < datetime.today():
                raise Exception(
                    'Cannot approve share extension since its it past the requested expiration date. Please reject this share and submit a new share request'
                )

The dataset expiration settings are defined in the fixture dataset_with_expiration_with_autoapproval

        enableExpiration=True,
        expirySetting='Monthly',
        expiryMinDuration=1,
        expiryMaxDuration=3,
    )

The issue might be in backend/dataall/modules/shares_base/services/share_object_service.py:327 where we calculate the expiration date. We should be trying to set it to October 31 not to September 30.

            # in this case expirationPeriod = 1, expirySetting = Monthly
            monthlyCalculatedDate = currentDate + relativedelta(months=expirationPeriod - 1)
            monthEndDay = calendar.monthrange(monthlyCalculatedDate.year, monthlyCalculatedDate.month)[1]
            shareExpiryDate = datetime(monthlyCalculatedDate.year, monthlyCalculatedDate.month, monthEndDay)

Today is Sept 30. the line to calculate the end of month date is wrong as the delta is 0 so monthlyCalculatedDate = 30/09/2024 + relativedelta(months=1 - 1) = 30/09/2024

How to Reproduce

Run tests as it is done automatically in our PRs. One example of the failure is in this PR: https://github.com/data-dot-all/dataall/actions/runs/11102824595/job/30843468276?pr=1573

Expected behavior

No response

Your project

No response

Screenshots

No response

OS

na

Python version

na

AWS data.all version

2.6.0

Additional context

No response

[dlpzx]

@TejasRGitHub I think we can use your input here. Why is there a - 1 in the calculation of the time deltas?

[TejasRGitHub]

Hi @dlpzx , Thanks for spotting this issue.

The -1 was added to make sure that we are not allocating more than required months on the expiration.

For example,
If someone requested share expiration for 1 month and the current date was 2nd Sept, then in that case monthlyCalculatedDate = currentDate + relativedelta(months=expirationPeriod - 1) would yield 0 and thus the end of month will be set as expiration.
With out -1, the expiration date will be set to end of Oct .

But with the bug you just raised I got to see how this is problematic.

Proposed change in share expiration:

In this scenario ( represented in the bug filed above ) , if a user requests a share at the end of the month, then the user should be granted expiration till the end of the next month.
But in the scenario when the user is requesting for 1 month share expiration and the current date is 1st Sept , he should be given expiration till the end of the month

Calculation can be modified to :

if currentDate > Total Days in Month / 2: 
    monthlyCalculatedDate = currentDate + relativedelta(months=expirationPeriod)
else:
    monthlyCalculatedDate = currentDate + relativedelta(months=expirationPeriod - 1)

[dlpzx]

I am implementing this fix; with the proposal the behavior is that if we are in the first half of the month then we extend until end of month and if we are in the second half of the month we extend until end of next month?

[TejasRGitHub]

I am implementing this fix; with the proposal the behavior is that if we are in the first half of the month then we extend until end of month and if we are in the second half of the month we extend until end of next month?

Yes. you are right

[TejasRGitHub]

@dlpzx , also for this one,

-xxxxxx----
Checking the logs of getShareObject after the request is submitted:
'expiryDate': '2024-09-30 00:00:00', 'requestedExpiryDate': '2024-09-30 00:00:00' ---> which means that share.requestedExpiryDate < datetime.today() is True so the test falls into this block from the share_object_service:

            if dataset.enableExpiration and share.requestedExpiryDate and share.requestedExpiryDate < datetime.today():
                raise Exception(
                    'Cannot approve share extension since its it past the requested expiration date. Please reject this share and submit a new share request'
                )

-xxxxx----

Just wanted to clarify that no changes needed in this . In my previous message I erred and also proposed changes in this but it it not needed.