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

New features for csv import plugin #14330

Merged
merged 6 commits into from Oct 6, 2018
Merged

New features for csv import plugin #14330

merged 6 commits into from Oct 6, 2018

Conversation

aroralakshya
Copy link
Contributor

@aroralakshya aroralakshya commented May 26, 2018

Following new features have been added in the csv import plugin:

  1. For a new import, user can now directly enter the table or database name at the time of import
  2. Partial import. User can enter how many rows they want to import.
    Signed-Off-By: Lakshay arora arora.lakshya123@gmail.com

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests

@aroralakshya aroralakshya changed the title Enh 2 New features for csv import plugin May 26, 2018
js/import.js Outdated
@@ -60,6 +60,8 @@ AJAX.registerOnload('import.js', function () {
var radioLocalImport = $('#radio_local_import_file');
var radioImport = $('#radio_import_file');
var fileMsg = '<div class="error"><img src="themes/dot.gif" title="" alt="" class="icon ic_s_error" /> ' + PMA_messages.strImportDialogMessage + '</div>';
var wrongTblNameMsg = '<div class="error"><img src="themes/dot.gif" title="" alt="" class="icon ic_s_error" />Please enter a valid table name</div>';
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 make these strings translatable; see the line above for an example. Basically, we put the actual string in https://github.com/phpmyadmin/phpmyadmin/blob/master/js/messages.php and then reference it from here; that way the translators can also put it in their own language.

js/functions.js Outdated
data: params,
success: function (data) {
if (data.success) {
$("<h3>You can change the table structure here</h3>" + data.message).insertAfter($("#new_csv_db_name").parent());
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 you forgot to make this string translatable.

…csv file.

Signed-Off-By: Lakshay arora <arora.lakshya123@gmail.com>
Following new features have been added in the csv import plugin:
1. For a new import, user can now directly enter the table or database name at the time of import
2. Partial import. User can enter how many rows they want to import.
Signed-Off-By: Lakshay arora <arora.lakshya123@gmail.com>
1. For a new import, user can now directly enter the table or database name at the time of import
2. Partial import. User can enter how many rows they want to import.
3. Show the form to edit table structure after import(undecided).

Signed-Off-By: Lakshay arora <arora.lakshya123@gmail.com>
@aroralakshya
Copy link
Contributor Author

Applied changes and solved merge conflicts. I'll work on tests as soon as travis finishes.

…import

Signed-Off-By: Lakshay arora <arora.lakshya123@gmail.com>
Signed-Off-By: Lakshay arora <arora.lakshya123@gmail.com>
@ibennetch
Copy link
Member

ibennetch commented Aug 8, 2018

I think you forgot to make <h3>You can change the table structure here</h3> translatable here, other than that I think you got all of the requested changes.

@codecov
Copy link

codecov bot commented Aug 9, 2018

Codecov Report

Merging #14330 into master will increase coverage by 0.33%.
The diff coverage is 84.37%.

@@             Coverage Diff              @@
##             master   #14330      +/-   ##
============================================
+ Coverage      49.7%   50.03%   +0.33%     
- Complexity    13871    14447     +576     
============================================
  Files           498      498              
  Lines         66803    67342     +539     
============================================
+ Hits          33204    33696     +492     
- Misses        33599    33646      +47

@aroralakshya
Copy link
Contributor Author

Done

js/import.js Outdated
}
if($("#text_csv_new_db_name").length > 0) {
var newDBName = $("#text_csv_new_db_name").val();
if(newDBName.length > 0 && $.trim(newDBName).length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These linting errors are in many of my PRs, it says it can be fixed using --fix option, how can I use this, I even asked this on the mailing list but got no reply.

Copy link
Member

Choose a reason for hiding this comment

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

Probably ./node_modules/.bin/eslint js/import.js --fix will do what you're after. Sorry I missed your email!

Copy link
Member

Choose a reason for hiding this comment

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

And if you do not have the node_modules directory, you need to run the yarn install command

Signed-off-by: Lakshay arora <arora.lakshya123@gmail.com>
@aroralakshya
Copy link
Contributor Author

I've fixed lint and other tests, but I don't understand this new selenium error?

@MauricioFauth MauricioFauth merged commit 19cbb34 into phpmyadmin:master Oct 6, 2018
@MauricioFauth
Copy link
Member

Merged, thanks for your contribution!

@MauricioFauth MauricioFauth self-assigned this Oct 6, 2018
@MauricioFauth MauricioFauth added this to the 5.0.0 milestone Oct 6, 2018
MauricioFauth added a commit that referenced this pull request Oct 6, 2018
Signed-off-by: Maurício Meneghini Fauth <mauriciofauth@gmail.com>
wincdev pushed a commit to wincdev/phpmyadmin that referenced this pull request Oct 8, 2018
Signed-off-by: Maurício Meneghini Fauth <mauriciofauth@gmail.com>
Signed-off-by: Vincent Horváth <win9400@gmail.com>
wincdev pushed a commit to wincdev/phpmyadmin that referenced this pull request Oct 8, 2018
Following new features have been added in the csv import plugin:
1. For a new import, user can now directly enter the table or database name at the time of import
2. Partial import. User can enter how many rows they want to import.

Signed-Off-By: Lakshay arora <arora.lakshya123@gmail.com>
wincdev pushed a commit to wincdev/phpmyadmin that referenced this pull request Oct 8, 2018
Signed-off-by: Maurício Meneghini Fauth <mauriciofauth@gmail.com>
Signed-off-by: Vincent Horváth <win9400@gmail.com>
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.

None yet

3 participants