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

add invalid and error details to vcard import report #9605

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

johndoh
Copy link
Contributor

@johndoh johndoh commented Aug 25, 2024

for #9591

currently there are 2 silent error cases on vcard import, invalid (fails roundcube contact validation) and error (its valid but i didnt insert for some reason e.g. plugin prevented it) this adds messages about those to the screen.

Untitled-1

Copy link
Member

@pabzm pabzm left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! I really appreciate your attention to issues related to "your" topic!

program/actions/contacts/import.php Outdated Show resolved Hide resolved
Copy link
Member

@pabzm pabzm left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@pabzm pabzm requested a review from alecpl September 16, 2024 13:16
Copy link
Member

@alecpl alecpl left a comment

Choose a reason for hiding this comment

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

Approved with some comments.

@@ -162,6 +162,8 @@ $messages['importwait'] = 'Importing, please wait...';
$messages['importformaterror'] = 'Import failed! The uploaded file is not a valid import data file.';
$messages['importconfirm'] = '<b>Successfully imported $inserted contacts</b>';
$messages['importconfirmskipped'] = '<b>Skipped $skipped existing entries</b>';
$messages['importconfirminvalid'] = '<b>Skipped $invalid invalid entries</b>';
$messages['importconfirmerrors'] = '<b>Failed to import $errors valid contacts</b>';
Copy link
Member

Choose a reason for hiding this comment

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

We should probably get rid of HTML tags from all these three messages.

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 think removing the <b> tags would be a breaking change because the formatting should be moved to the skin to do this properly. I can work on this in a separate PR.

program/actions/contacts/import.php Outdated Show resolved Hide resolved
@pabzm
Copy link
Member

pabzm commented Nov 11, 2024

@alecpl I'll merge this in one week if nothing happens until then, since you left your approval already and the only open topic is very minor.

@alecpl
Copy link
Member

alecpl commented Nov 19, 2024

Feel free to merge, close the related ticket (assigning milestone) if appropriate and update the changelog.

@pabzm
Copy link
Member

pabzm commented Nov 20, 2024

@johndoh Would you add a line to the Changelog? (I can do it as well but would prefer to have it as part of the PR.)

@pabzm
Copy link
Member

pabzm commented Nov 20, 2024

/remind me to merge on Monday

Copy link

@pabzm set a reminder for 11/25/2024

@johndoh
Copy link
Contributor Author

johndoh commented Nov 20, 2024

I've added a changelog entry. If you don't like the text or its in the wrong place its no problem.

Copy link

👋 @pabzm, merge

@github-actions github-actions bot removed the reminder label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants