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.

Sign up for GitHub

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

Jump to bottom

[full-ci] Fix inconsistencies in share expiry dates #6084

Merged
merged 5 commits into from
Dec 13, 2021
Merged

[full-ci] Fix inconsistencies in share expiry dates #6084

merged 5 commits into from
Dec 13, 2021

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Dec 2, 2021 •
edited
Loading

Description

  • Share expiry dates now always refer to the end of the given day. This change allows users to select the current day as expiry date.
  • Displayed expiry dates have been aligned to ensure their consistency.
  • Existing expiry dates for public links can now be removed again.
  • We now use the Luxon DateTime object more consistently across the code base (replacing JavaScript's new Date()).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@JammingBen JammingBen changed the title Fix inconsistencies for share expiry dates Fix inconsistencies in share expiry dates Dec 2, 2021
@JammingBen JammingBen force-pushed the fix-date-inconsistencies branch from 1f687be to 0c7a443 Compare December 2, 2021 10:44
@owncloud owncloud deleted a comment from update-docs bot Dec 2, 2021
@JammingBen JammingBen marked this pull request as ready for review December 2, 2021 10:45
@JammingBen JammingBen self-assigned this Dec 2, 2021
@JammingBen JammingBen added Status:Needs-Review Needs review from a maintainer Type:Bug Something isn't working labels Dec 2, 2021
kulmann
kulmann approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

🕐 🕙 🕥 🕦 🥳

packages/web-app-files/src/components/SideBar/Links/PublicLinks/LinkEdit.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Links/PublicLinks/LinkEdit.vue Outdated
expireDate: DateTime.fromJSDate(this.expireDate)
.setLocale(this.$language.current)
.endOf('day')
.toFormat("yyyy-MM-dd'T'HH:mm:ssZZZ"),
Copy link
Contributor

Choose a reason for hiding this comment

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

You chose this format because Reva is not compatible with the standard ISO format, right? If so, we should create tickets in Reva to fix that and in Web to be able to track it. What do you think?

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've created cs3org/reva#2322. I'll create the corresponding Web issue as well once this has been acknowledged and accepted from their side.

@JammingBen
Copy link
Contributor Author

JammingBen commented Dec 3, 2021 •
edited
Loading

Updated the PR, thanks to the tests I found out that a) removing expiry dates from people shares did not work because of my changes and b) removing expiry dates from public links did not work at all in the past. Both should be fixed now.

Edit: I'm thinking about adding an integration test for b), as we have one for a) as well 🤔

@JammingBen JammingBen requested a review from kulmann December 3, 2021 10:58
@kulmann
Copy link
Contributor

kulmann commented Dec 3, 2021

Updated the PR, thanks to the tests I found out that a) removing expiry dates from people shares did not work because of my changes and b) removing expiry dates from public links did not work at all in the past. Both should be fixed now.

Edit: I'm thinking about adding an integration test for b), as we have one for a) as well 🤔

👍 for the integration test for (b) - nice findings! 😍

pascalwengerter
pascalwengerter reviewed Dec 3, 2021
View reviewed changes
packages/web-app-files/src/components/SideBar/Shares/Collaborators/CollaboratorsEditOptions.vue
date.setDate(new Date().getDate() + 1)

return date
return DateTime.now().setLocale(this.$language.current).toJSDate()
Copy link
Contributor

Choose a reason for hiding this comment

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

@kulmann this makes changes/rebase in #6039 necessary, too from my understanding

@pascalwengerter pascalwengerter changed the title Fix inconsistencies in share expiry dates [full-ci] Fix inconsistencies in share expiry dates Dec 8, 2021
@pascalwengerter pascalwengerter force-pushed the fix-date-inconsistencies branch from 1f0d23a to fc12c00 Compare December 8, 2021 11:56
@pascalwengerter
Copy link
Contributor

Rebased on current master and running as full-ci for once to see what's crackin'

@JammingBen
Copy link
Contributor Author

Thx @pascalwengerter ! I already started implementing integration tests for this, my plan still is to finish that this week.

@pascalwengerter
Copy link
Contributor

Thx @pascalwengerter ! I already started implementing integration tests for this, my plan still is to finish that this week.

Ok cool, was under the impression this one either needed a fix in Reva or a green CI to be merged :D but more tests it is, nice. Maybe we'll have to update the acceptance tests too/even have unexpected passes

@pascalwengerter
Copy link
Contributor

@JammingBen could you take a look at #5663 when you're in the process again? Maybe that's related&easy enough to fix it in this PR, too

@JammingBen JammingBen removed the Status:Needs-Review Needs review from a maintainer label Dec 10, 2021
@JammingBen
Copy link
Contributor Author

These integration tests man, I think I lost quite some brain cells on the way. Anyway, those should work now. But I think the failings smoke tests are another issue I have to look into.

#5663 seems to be fixed already (see #5806).

@pascalwengerter
Copy link
Contributor

These integration tests man, I think I lost quite some brain cells on the way. Anyway, those should work now. But I think the failings smoke tests are another issue I have to look into.

#5663 seems to be fixed already (see #5806).

Flaky some tests are on us, no worries

@JammingBen JammingBen added the Status:Needs-Review Needs review from a maintainer label Dec 10, 2021
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.5% 90.5% Coverage
0.0% 0.0% Duplication

@ownclouders
Copy link
Contributor

Results for oCISSharingPublic https://drone.owncloud.com/owncloud/web/20890/65/1
The following scenarios passed on retry:

  • webUISharingPublicManagement/shareByPublicLink.feature:19

@kulmann kulmann merged commit 2954889 into master Dec 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix-date-inconsistencies branch December 13, 2021 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kulmann kulmann kulmann approved these changes

@fschade fschade Awaiting requested review from fschade

@pascalwengerter pascalwengerter Awaiting requested review from pascalwengerter

Assignees

@JammingBen JammingBen

Labels
Status:Needs-Review Needs review from a maintainer Type:Bug Something isn't working
Projects
No open projects
Web Support Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JammingBen @kulmann @pascalwengerter @ownclouders