Skip to content

Revert "fix: use built now function instead of manually creating date" - #4609

Closed
hamza221 wants to merge 1 commit into
mainfrom
fix/invalid-rev
Closed

hamza221 wants to merge 1 commit into
mainfrom
fix/invalid-rev

Conversation

@hamza221

@hamza221 hamza221 commented Aug 11, 2025 •

Copy link
Copy Markdown
Contributor

discovered in #4521
reverts #4573

According to the pre-existing logic :
https://github.com/nextcloud/contacts/blob/fix/invalid-rev/src/services/checks/invalidREV.js#L24-L30
vcardtime is expected for v3 contacts and icaltime is expected for v4
With the reverted commit that was not respected. According to that logic even the repaired rev is wrong.

creating rev from unix timestamp or js date are both supported by the library so I don't see anything wrong with the old method imo

@hamza221 hamza221 self-assigned this Aug 11, 2025
@hamza221
hamza221 requested a review from GVodyanov as a code owner August 11, 2025 14:54
@hamza221 hamza221 added 3. to review Waiting for reviews regression Regression of a previous working feature labels Aug 11, 2025
This reverts commit d255c26.

Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
@codecov

codecov Bot commented Aug 11, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 10.58%. Comparing base (8075b3c) to head (f2fd8ac).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
src/services/checks/invalidREV.js 0.00% 3 Missing ⚠️
src/store/contacts.js 0.00% 3 Missing ⚠️
src/views/Contacts.vue 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4609      +/-   ##
============================================
+ Coverage     10.56%   10.58%   +0.02%     
  Complexity      265      265              
============================================
  Files           123      123              
  Lines          5767     5775       +8     
  Branches       1262     1262              
============================================
+ Hits            609      611       +2     
- Misses         5035     5041       +6     
  Partials        123      123              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ChristophWurst ChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense

@hamza221

Copy link
Copy Markdown
Contributor Author

/backport to stable7.2

@SebastianKrupinski

Copy link
Copy Markdown
Contributor

FYI, doing this will break contacts saving. The vcardtime does not seem to work properly in the updated ical.js.

I would recommend fixing the check instead, since its a 5 year old patch for a bug that probably does not exist anymore

@hamza221
hamza221 marked this pull request as draft August 14, 2025 12:27
@hamza221

Copy link
Copy Markdown
Contributor Author

fixed in #4634

@hamza221 hamza221 closed this Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews backport-request regression Regression of a previous working feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants