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

Only show the database login fields when necessary #2615

Conversation

antoine2711
Copy link
Member

The database login field’s visibility are now controlled by CSS styling.

Choosing sqlite will only show the database field.

Regards,
Antoine

The database login field’s visibility is now controled by CSS styling.
@antoine2711 antoine2711 linked an issue May 11, 2020 that may be closed by this pull request
Change field name from Database to Database file.
Use full db name instead of diminutives for the CSS classes.
Added translation to the Input placeholders.
remerge Database field and DatabaseFile Field like before.
Created Refine.DatabaseSourceUI.prototype._updateDatabaseType(databaseType)
<option value="mysql" selected="selected"">MySQL</option>
Fixed typo. (<option value="mysql" selected="selected">MySQL</option>)
New default connection name value, translation of it, changing cssClassName from options to dbtype-options, adding the prefix "dbt-" to the db types and fix the changing of placeholder databaseName/databaseFileName when neccessary.
Fix issue with « saved connections » and added 2 defaults values for dbHost and dbType.
<option value="mysql" selected="selected">MySQL</option>
@antoine2711
Copy link
Member Author

antoine2711 commented May 14, 2020

@wetneb & @thadguidry: so, I guess it's pretty done.

  • Each field can be show/hidden with 1 class name (dbtype value prefixed with "dbt-") for every dbtype
  • Placeholders of input fields are now all getting they values from i18n.
  • Edit Connections now works with show/hide fields
  • Default value for dbtype is now queried from the HTML, and used for the first connections or « Reset Connections ».
  • Default value for dbHost: it is now localhost everywhere, stored in a property of the object DatabaseSourceUI.
  • If issue « Access to a JS object on the client side with all the preferences, that would not be updated (frequently) Access to a JS object on the client side with all the preferences, that would not be updated (frequently) #2619 » is implemented, it will be easy for a user to create two preferences, defaultDatabaseType and defaultDatabaseHost, and they would be used by the interface.
  • Default connection name is now given by i18n, so it changes in the user language. ("database-source/connectionNameDefaultValue": "New Connection Name").

If you fell there is something else to do, let me know.

Regards,
Antoine

@antoine2711
Copy link
Member Author

antoine2711 commented May 19, 2020

@thadguidry: when you have the time, I really think you should check this enhancement. I actually think it's more a bug fix for interface. Anyway, I think we should put it in v3.4, since it's to make SQLite very simple to use.

And now if we have « Access to a JS object on the client side with all the preferences, that would not be updated (frequently) #2619 », we could even store a default value for database server, and database type.

Regards,
Antoine

Co-authored-by: Thad Guidry <thadguidry@gmail.com>
@antoine2711 antoine2711 requested a review from wetneb May 20, 2020 19:40
@thadguidry
Copy link
Member

thadguidry commented May 20, 2020

@antoine2711 It would be really nice to have both Travis and Appveyor, actually test create the sqlite and test... as well as fix the travis-pgsql.sql failure on Appveyor. Whenever you have the time. :-)

UPDATE: Hmm, looks like a syntax error, we probably don't need that USE line any longer in extensions\database\tests\conf\travis-mysql.sql ? Since we already state on Travis config that we are going to use the test_db through the before_install Line 26 here: https://github.com/OpenRefine/OpenRefine/blob/master/.travis.yml#L26

And why is this comment still here? https://github.com/OpenRefine/OpenRefine/blob/master/extensions/database/tests/conf/travis_tests.xml#L32 Did something not get put back into place correctly? a6424e5 Groups run nothing? Is that a problem?

Sorry for exploding a bunch of questions here for you and @wetneb to dig into, to ensure integrity on our database extension testing on Travis and Appveyor is healthy again and working correctly fully. Thanks!

@antoine2711
Copy link
Member Author

@antoine2711 It would be really nice to have both Travis and Appveyor, actually test create the sqlite and test... as well as fix the travis-pgsql.sql failure on Appveyor. Whenever you have the time. :-)

Sure, I could learn how to do that and implement it. I wouldn't be fast, though. I will follow the discussion here. If you have links to docs on that matter, please share.

Would you like me to create a new issue: « Fix Travis and Appveyor tests for databases ». Would that be good? I think it could deserve a dedicated attention.

Regards,
Antoine

@thadguidry
Copy link
Member

thadguidry commented May 20, 2020

@antoine2711 yes please, new issue with my facts above. Create the new issue, use a new branch, and iterate on it until db tests are working well on Appveyor and Travis. Looks like mostly just some .yml file updates and .sql file updates in various places are all that's needed. You can see the previous pqsql and mysql as examples...oh wait, we have sqlite already by that fellow a few months ago...so just ensure Appveyor and Travis are indeed testing the DB extension against those 3, postgres, mysql, sqlite properly.

@antoine2711
Copy link
Member Author

From another thread, « might » be fixed here:

snussik: By the way, don’t know if it is a bug or feature but if I choose “SQLite” engine and enter any string to database textfield, “test” button shows “Ok”.

@antoine2711
Copy link
Member Author

@antoine2711 yes (…) use a new branch, and iterate on it until db tests are working well on Appveyor and Travis. Looks like mostly just some .yml file updates and .sql file updates in various places are all that's needed. You can see the previous pqsql and mysql as examples… oh wait, we have sqlite already by that fellow a few months ago… so just ensure Appveyor and Travis are indeed testing the DB extension against those 3, postgres, mysql, sqlite properly.

@thadguidry: I don't understand where it fails. If I do: ./refine test, I get this at the end:

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for OpenRefine 3.4-SNAPSHOT:
[INFO] 
[INFO] OpenRefine ......................................... SUCCESS [  0.696 s]
[INFO] OpenRefine - main .................................. SUCCESS [ 37.771 s]
[INFO] OpenRefine - server ................................ SUCCESS [  0.391 s]
[INFO] OpenRefine - extensions ............................ SUCCESS [  0.006 s]
[INFO] OpenRefine - Jython extension ...................... SUCCESS [  3.744 s]
[INFO] OpenRefine - Wikidata extension .................... SUCCESS [  5.949 s]
[INFO] OpenRefine - Database extension .................... SUCCESS [  1.107 s]
[INFO] OpenRefine - Gdata extension ....................... SUCCESS [  1.875 s]
[INFO] OpenRefine - PC-axis extension ..................... SUCCESS [  0.062 s]
[INFO] OpenRefine - Phonetic clustering extension ......... SUCCESS [  1.152 s]
[INFO] OpenRefine - packaging ............................. SUCCESS [  0.846 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  53.817 s
[INFO] Finished at: 2020-05-21T03:07:38-04:00
[INFO] ------------------------------------------------------------------------
Mac-de-Antoine:OpenRefine antoine$ 

Where can I see the failing you are talking about?

Regards,
Antoine

@thadguidry
Copy link
Member

thadguidry commented May 21, 2020

On GitHub. The PR when viewed will show near the bottom the checks passing/failing.
image

When you click on Details blue link it will take you to Appveyor or Travis and there you can see the logs from each respectively:
image

Our tests should actually use those various SQL engine instances that we have wired up to spin up in the CI (Continuous Integration) vendors like Appveyor and Travis. I am unsure if all the DB extension tests are using those instances...and if we are in fact still testing against those 3 DB instances we spin up on Travis and Appveyor. So double-check all of that. I want to make sure we didn't accidentally turn off testing for some, or skipping tests for some (we did skip some things in the past because of problems with Appveyor but should no longer be skipping tests AT ALL, so that we can ensure regression testing properly going forward with all these changes and expanding our contributor base such as with GSoC and yourself ;-) I.E. I don't want our quality to go down and for us to get lazy.

@tfmorris
Copy link
Member

The first thing to fix is to make sure that the run gets marked as failed when it hits an error like this.

@wetneb
Copy link
Member

wetneb commented May 22, 2020

@thadguidry the errors you mention are unrelated to this PR. Yes it would be worth cleaning up this SQL script but this is completely independent from these changes.

Copy link
Member

@wetneb wetneb left a comment

Choose a reason for hiding this comment

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

There seems to be an issue with database path validation for the SQLite set-up. If I try to connect to a SQLite database via its full path, I get the error Connection Name Input Error: Illegal Character in Input. Only [a-zA-Z0-9._-] Allowed, which does not happen on master branch.

Some of the placeholders added in the form inputs seem redundant with the field labels. It does not feel natural to me to have all these "Enter …" prefixes there (these are text fields - the user already knows that they need to enter something there). Also I would not capitalize every word there - these are not titles, so let's not use title case for them.

For instance having "Enter Database" as placeholder for a "Database" field is not helpful. I would prefer to have something like "database name" instead, since that is more precise - but to be honest this could just be the label of the field itself.

@antoine2711
Copy link
Member Author

antoine2711 commented May 22, 2020

There seems to be an issue with database path validation for the SQLite set-up. If I try to connect to a SQLite database via its full path, I get the error Connection Name Input Error: Illegal Character in Input. Only [a-zA-Z0-9._-] Allowed, which does not happen on master branch.

@wetneb: oh! I never saw that. My first impression, is that some data in other fields (hidden for sqlite) are containing bad data. I'll check it.

Some of the placeholders added in the form inputs seem redundant with the field labels. It does not feel natural to me to have all these "Enter …" prefixes there (these are text fields - the user already knows that they need to enter something there). Also I would not capitalize every word there - these are not titles, so let's not use title case for them.

For instance having "Enter Database" as placeholder for a "Database" field is not helpful. I would prefer to have something like "database name" instead, since that is more precise - but to be honest this could just be the label of the field itself.

I hear you, and I share your opinion, but, before going further here, I must say that I didn't write this stuff, all of that was previously there in the HTML (the english version). I only factorized it with i18n code.

The french version, on the other hand, is all of my writing. So, in english, I can remove all capitals, except the first, and change « database name ». For the rest, I wouldn't put now too much time, as the i18n data is now exposed, and easier to follow/clean.

Regards,
Antoine

Fix $( "#databaseHost" ).val(self._defaultDatabaseHost) to be set before calling self._updateDatabaseType(self._defaultDatabaseType) that may change it.
Copy link
Member

@wetneb wetneb left a comment

Choose a reason for hiding this comment

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

Ok thanks, I am happy with cleaning up the labels in a follow-up PR.

@antoine2711
Copy link
Member Author

antoine2711 commented May 23, 2020

@wetneb: It's fixed. The problem was the name of the connection, it can't have space. So I changed the default to New_Connection_Name. Much better than 127.0.0.1 that was there before this PR.

What do we do about the test button which always succeed, even when the path is bad? I think another issue should be opened for that.

Regards,
Antoine

Removed field name text since it's populated from i18n and fixed the default connection name so that it has no space in it.
@antoine2711
Copy link
Member Author

antoine2711 commented May 23, 2020

@thadguidry: per your request, here's the new issue to bring back the Travis and Appveyor tests.
« Bring back Travis & Appveyor tests for Database Import #2637 »

Don't hesitate to modify it, of course. I tried to edit it a bit, but still far from perfect.

Regards,
Antoine

@antoine2711 antoine2711 requested a review from wetneb May 24, 2020 03:05
Copy link
Member

@wetneb wetneb 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 very much for that!

@wetneb wetneb merged commit 356c763 into OpenRefine:master May 30, 2020
@antoine2711
Copy link
Member Author

Thank you very much for that!

You're welcome. It was a pleasure working with you on that. Thanks for your support and guidance.

Best Regards,
Antoine

wetneb added a commit that referenced this pull request May 30, 2020
* Add a Show/Hide left panel

Add a Show/Hide left Facets-Undo/Redo panel.

* Update summary-bar.js

Simplify the code for the "hide-left-panel-button".

* Remove the gap on the left of the button

Remove the gap on the left of the button and align left with table.

* Translated using Weblate (Japanese)

Currently translated at 100.0% (740 of 740 strings)

Translation: OpenRefine/Translations
Translate-URL: https://hosted.weblate.org/projects/openrefine/translations/ja/

* Translated using Weblate (Japanese)

Currently translated at 100.0% (171 of 171 strings)

Translation: OpenRefine/wikidata
Translate-URL: https://hosted.weblate.org/projects/openrefine/wikidata/ja/

* Translated using Weblate (Japanese)

Currently translated at 100.0% (47 of 47 strings)

Translation: OpenRefine/gdata
Translate-URL: https://hosted.weblate.org/projects/openrefine/gdata/ja/

* Updated  showhide (collapse) arrow image

* Bolden the border dark-blue

Bolden the border dark-blue

* Changed from SPAN to A tag

Changed from SPAN to A tag.

* Update project.less

Minor change: left: 22px;

* Add a Show/Hide left panel

Add a Show/Hide left Facets-Undo/Redo panel.

* Update summary-bar.js

Simplify the code for the "hide-left-panel-button".

* Remove the gap on the left of the button

Remove the gap on the left of the button and align left with table.

* Updated  showhide (collapse) arrow image

* Bolden the border dark-blue

Bolden the border dark-blue

* Changed from SPAN to A tag

Changed from SPAN to A tag.

* Update project.less

Minor change: left: 22px;

* Changed cell.error to cell.errorMessage & added help data. (#2628)

* Changed cell.error to cell.errorMessage & added help data.

Changed cell.error to cell.errorMessage and added the informations into the internal help system.

* FR Text correction

* HU Fix text

3 instead of 2.

* The show/hide button now changes side

The show/hide button now changes side.

* Update project.js

Removed unneeded self.

* Update summary-bar.js

Removed unneeded self.

* [Security] Bump jackson.version from 2.9.10 to 2.11.0

Bumps `jackson.version` from 2.9.10 to 2.11.0.

Updates `jackson-databind` from 2.9.10 to 2.11.0
- [Release notes](https://github.com/FasterXML/jackson/releases)
- [Commits](https://github.com/FasterXML/jackson/commits)

Updates `jackson-annotations` from 2.9.10 to 2.11.0
- [Release notes](https://github.com/FasterXML/jackson/releases)
- [Commits](https://github.com/FasterXML/jackson/commits)

Updates `jackson-core` from 2.9.10 to 2.11.0
- [Release notes](https://github.com/FasterXML/jackson-core/releases)
- [Commits](FasterXML/jackson-core@jackson-core-2.9.10...jackson-core-2.11.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

* Translated using Weblate (Russian)

Currently translated at 91.0% (674 of 740 strings)

Translation: OpenRefine/Translations
Translate-URL: https://hosted.weblate.org/projects/openrefine/translations/ru/

* Only show the database login fields when necessary (#2615)

* DB login fields visibility controled by CSS

The database login field’s visibility is now controled by CSS styling.

* Change field name from Database to Database file.

Change field name from Database to Database file.

* Use full db name as CSS classes

Use full db name instead of diminutives for the CSS classes.

* Added translation to placeholders

Added translation to the Input placeholders.

* Undo a change, remerge Database field & File

remerge Database field and DatabaseFile Field like before.

* Created DatabaseSourceUI._updateDatabaseType(dbType)

Created Refine.DatabaseSourceUI.prototype._updateDatabaseType(databaseType)

* Make MySQL the default database

<option value="mysql" selected="selected"">MySQL</option>

* Update database-import-form.html

Fixed typo. (<option value="mysql" selected="selected">MySQL</option>)

* New default connection name value

New default connection name value, translation of it, changing cssClassName from options to dbtype-options, adding the prefix "dbt-" to the db types and fix the changing of placeholder databaseName/databaseFileName when neccessary.

* Fix issue with « saved connections »

Fix issue with « saved connections » and added 2 defaults values for dbHost and dbType.

* Default DB back to MySQL.

<option value="mysql" selected="selected">MySQL</option>

* Update extensions/database/module/langs/translation-en.json

Co-authored-by: Thad Guidry <thadguidry@gmail.com>

* Better default with « mysql »

* Fix sqlite #databaseHost before calling self._updateDatabaseType()

Fix $( "#databaseHost" ).val(self._defaultDatabaseHost) to be set before calling self._updateDatabaseType(self._defaultDatabaseType) that may change it.

* Removed field name & fixed default connection name

Removed field name text since it's populated from i18n and fixed the default connection name so that it has no space in it.

* Little update to placeholder text

Co-authored-by: Thad Guidry <thadguidry@gmail.com>

* Resize the grid UI as well

Co-authored-by: Isao Matsunami <isao.matsunami@gmail.com>
Co-authored-by: Hosted Weblate <hosted@weblate.org>
Co-authored-by: Thad Guidry <thadguidry@gmail.com>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tom Morris <tfmorris@gmail.com>
Co-authored-by: Artem <KovalevArtem.ru@gmail.com>
Co-authored-by: Antonin Delpeuch <antonin@delpeuch.eu>
@wetneb wetneb added this to the 3.5 milestone May 30, 2020
@antoine2711 antoine2711 deleted the issue-1951-show-pertinent-textfields-import-db branch December 20, 2021 08:57
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.

SQLite database connector
4 participants