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

Import file FileAction 28 compatibility #3669

Merged
merged 1 commit into from
Nov 1, 2023
Merged

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Oct 18, 2023

fixes #3621

  • Add initi.ts file
  • addInitScript on LoadContactsFilesActions.php
  • Migrate files-action.js to the new api
  • Check documentation how to migrate OCA.Files.fileActions.setDefault(mime, name)

@GretaD GretaD added the 2. developing Work in progress label Oct 18, 2023
@GretaD GretaD self-assigned this Oct 18, 2023
@GretaD GretaD marked this pull request as draft October 18, 2023 10:55
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (75f294f) 2.07% compared to head (bb0e8ae) 0.00%.
Report is 20 commits behind head on main.

❗ Current head bb0e8ae differs from pull request most recent head 5493404. Consider uploading reports for the commit 5493404 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              main   #3669      +/-   ##
==========================================
- Coverage     2.07%   0.00%   -2.08%     
- Complexity       0     261     +261     
==========================================
  Files           89      24      -65     
  Lines         5343     784    -4559     
  Branches      1491       0    -1491     
==========================================
- Hits           111       0     -111     
+ Misses        5114     784    -4330     
+ Partials       118       0     -118     
Files Coverage Δ
lib/Listener/LoadContactsFilesActions.php 0.00% <0.00%> (ø)

... and 112 files with indirect coverage changes

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

@GretaD GretaD requested a review from skjnldsv October 19, 2023 13:48
src/files-action.js Outdated Show resolved Hide resolved
@GretaD GretaD force-pushed the migrate/contacts-newfile-api branch from 3b66493 to 809e787 Compare October 25, 2023 08:01
@skjnldsv skjnldsv removed their request for review October 25, 2023 08:32
@GretaD GretaD requested a review from ChristophWurst October 25, 2023 12:54
@GretaD GretaD added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 25, 2023
@GretaD GretaD marked this pull request as ready for review October 25, 2023 12:54
lib/Listener/LoadContactsFilesActions.php Outdated Show resolved Hide resolved
src/files-action.js Outdated Show resolved Hide resolved
src/files-action.js Outdated Show resolved Hide resolved
src/files-action.js Outdated Show resolved Hide resolved
src/files-action.js Outdated Show resolved Hide resolved
src/init.d.ts Outdated Show resolved Hide resolved
@GretaD GretaD requested a review from st3iny October 27, 2023 12:48
src/files-action.js Outdated Show resolved Hide resolved
@GretaD GretaD requested a review from ChristophWurst October 31, 2023 14:47
.jshintrc Outdated Show resolved Hide resolved
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

I tested it and it works no master and stable27. Please address the feedback regarding jshint.

@GretaD GretaD requested a review from st3iny November 1, 2023 10:29
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. I fixed some minor bugs and merged both if statements.

Please squash all commits and we are ready to merge :)

@st3iny st3iny dismissed ChristophWurst’s stale review November 1, 2023 14:32

Feedback was addressed.

@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish technical debt and removed 3. to review Waiting for reviews labels Nov 1, 2023
@st3iny st3iny added this to the v5.5.0 milestone Nov 1, 2023
Signed-off-by: greta <gretadoci@gmail.com>
@GretaD GretaD force-pushed the migrate/contacts-newfile-api branch from 20b4bee to 5493404 Compare November 1, 2023 15:24
@GretaD GretaD enabled auto-merge November 1, 2023 15:24
@GretaD GretaD merged commit 6cd8952 into main Nov 1, 2023
27 of 28 checks passed
@GretaD GretaD deleted the migrate/contacts-newfile-api branch November 1, 2023 15:30
if (method_exists(Util::class, 'addInitScript')) {
Util::addInitScript(Application::APP_ID, 'contacts-files-action');
} else {
Util::addScript(Application::APP_ID, 'contacts-files-action');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Util::addScript(Application::APP_ID, 'contacts-files-action');
Util::addScript(Application::APP_ID, 'contacts-files-action', 'files');

}
console.error('Unable to register vcf import action')
})
if (nextcloudVersionIsGreaterThanOr28) {
Copy link
Member

Choose a reason for hiding this comment

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

I am not even sure this is needed. Worse case this will register an unused action 🤷

id: name,
displayName: () => t('contacts', 'Import'),
default: DefaultType.DEFAULT,
mime,
Copy link
Member

Choose a reason for hiding this comment

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

FileAction doesn't have a mime

default: DefaultType.DEFAULT,
mime,
enabled: (nodes) => {
return nodes.every((node) => node.mime === mime && (node.permissions & Permission.READ))
Copy link
Member

Choose a reason for hiding this comment

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

Using some with the opposite check is faster as it fill stop at the first false return
Something like

Suggested change
return nodes.every((node) => node.mime === mime && (node.permissions & Permission.READ))
return nodes.some((node) => node.mime !== mime || (node.permissions & Permission.READ) === 0)

@skjnldsv
Copy link
Member

skjnldsv commented Nov 3, 2023

Added some comments @GretaD @st3iny

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import file FileAction 28 compatibility
4 participants