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

Fixed case if user removes all cycles #193

Closed
wants to merge 3 commits into from

Conversation

imblowfish
Copy link
Collaborator

Fixed #166

In this PR I added possibility to save empty array of cycles. Also I removed isMarkedFutureDays because now we using ion-datetime built-in limitation instead of custom checks

New test has been added #192

@imblowfish imblowfish added this to the v2.3.6 milestone Nov 23, 2023
@imblowfish imblowfish self-assigned this Nov 23, 2023
@imblowfish
Copy link
Collaborator Author

@IraSoro could you give me rights for peri-tests project or add test #192 by yourself?

Copy link
Owner

@IraSoro IraSoro left a comment

Choose a reason for hiding this comment

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

Read more about this #178 and this #155 .

Firstly, the previous solution did not fix the bug. Below I will describe the steps.
Secondly, you need to leave checking future cycles and editing the current period. The user must be able to edit the current cycle. I missed this in previous PR.

I think that you need to leave the ability to edit the current cycle and leave checking the marked future dates.

@IraSoro
Copy link
Owner

IraSoro commented Dec 1, 2023

Steps:

  1. We have
    image
  2. Click Mark button
    image
  3. Click Edit button
    image
  4. Dont mark the current date
    image
  5. Save
    image

@imblowfish
Copy link
Collaborator Author

imblowfish commented Dec 3, 2023

@IraSoro based on steps that you described, looke like you confused this issue with #171
In this issue we just fixing problem when user trying to remove all cycles, but can't do that, because we have datetimeRef value check. I fixed this problem and now you can remove all cycles from the storage.

The problem what you described above is fixed in this PR #196. I checked by myself and everything works fine. And also you can see, that out of 4 commits from #196, 3 are completely duplicates commits from the current PR. And in this PR you can see why we don't need #178 anymore. User should be able to remove all days from cycle, if we'll leave this check then user will not be able to remove the current cycle in case it not finished yet

@imblowfish
Copy link
Collaborator Author

I closed this PR because #196 has the same changs

@imblowfish imblowfish closed this Dec 10, 2023
@imblowfish imblowfish deleted the 166-fix-all-cycles-deletion branch December 10, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

You can't delete all cycles in the calendar
2 participants