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

Added alert message for extensions required #15643

Closed
wants to merge 1 commit into from

Conversation

kartik1000
Copy link
Contributor

@kartik1000 kartik1000 commented Dec 11, 2019

Signed-off-by: Kartik Kathuria kathuriakartik0@gmail.com

Description

.
This will add an alert message if the user is trying to import a file with zip extension or bzip2 extension that the required extensions must be present or enabled to import a zip or bzip2 file.

Fixes #15328

Before submitting pull request, please review the following checklist:

  • Make sure you have read our CONTRIBUTING.md document.
  • Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
  • Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
  • Every commit has a descriptive commit message.
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
  • Any new functionality is covered by tests.

js/import.js Outdated Show resolved Hide resolved
@williamdes williamdes added this to In progress in pull-requests via automation Dec 16, 2019
@williamdes williamdes moved this from In progress to Review in progress in pull-requests Dec 16, 2019
@williamdes williamdes changed the base branch from QA_4_9 to QA_5_0 December 26, 2019 19:38
@codecov
Copy link

codecov bot commented Jan 11, 2020

Codecov Report

Merging #15643 into QA_5_0 will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             QA_5_0   #15643      +/-   ##
============================================
- Coverage     53.05%   53.05%   -0.01%     
  Complexity    14156    14156              
============================================
  Files           482      482              
  Lines         62132    62134       +2     
============================================
  Hits          32965    32965              
- Misses        29167    29169       +2

@kartik1000
Copy link
Contributor Author

@williamdes, I have made the changes. Please review.

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Quite good, added some minor changes

libraries/classes/Display/Import.php Outdated Show resolved Hide resolved
libraries/classes/Display/Import.php Outdated Show resolved Hide resolved
templates/display/import/import.twig Outdated Show resolved Hide resolved
templates/display/import/import.twig Outdated Show resolved Hide resolved
@kartik1000
Copy link
Contributor Author

@williamdes I have made the changes. Please review.

pull-requests automation moved this from Review in progress to Reviewer approved Jan 12, 2020
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

💯

@williamdes
Copy link
Member

@kartik1000 You mentioned #14326 but I see no implementation for the export page

@kartik1000
Copy link
Contributor Author

Oh, I didn't read it was the same as one of the issues was even mentioned in the other. No , the alert is only for import.

@williamdes
Copy link
Member

Okay
@MauricioFauth can I please have a review ?

@kartik1000
Copy link
Contributor Author

@williamdes @MauricioFauth Can we do a review so that we can look to merge this as this was listed as a high priority issue?

@williamdes
Copy link
Member

@kartik1000 Can you add a bit of JS code to show the error only when compression is selected ?
And also add a back-end check ?

@kartik1000
Copy link
Contributor Author

@williamdes Ok I will add those checks :)

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

👍
Maybe you need yarn run js-lint --fix

{% if extension_bz2 == false %}
<div class="formelementrow" id="alert_message_for_bz2">
<p>
<strong>{% trans 'Please enable the php bz2 extension to import a php bz2 file else it will result in a blank screen' %}</strong>
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to fix the blank page instead of showing this message.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, a back-end check + message is required

pull-requests automation moved this from Reviewer approved to Review in progress Feb 24, 2020
@ibennetch ibennetch modified the milestones: 5.0.2, 5.0.3 Mar 21, 2020
@stale
Copy link

stale bot commented Jun 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 26, 2020
See: https://filext.com/file-extension/BZ2
See: https://filext.com/file-extension/ZIP

Co-authored-by: William Desportes <williamdes@wdes.fr>
Signed-off-by: Kartik Kathuria <kathuriakartik0@gmail.com>
pull-requests automation moved this from Review in progress to Done Aug 16, 2020
@MauricioFauth MauricioFauth self-assigned this Aug 16, 2020
@MauricioFauth
Copy link
Member

Closed by 51b04c9. Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
pull-requests
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants