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

Fixes issue add support for more datatypes in filters (3) #62

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

Conversation

thomas-topway-it
Copy link
Contributor

@thomas-topway-it thomas-topway-it commented Jun 30, 2023

Adds support for all default SMW datatypes and fixes use of predefined datatypes as property label. (e.g. [[Email::test@mail.com]], but also [[Number::123]]) --- see tables below for a precise reference between master and this pull request.

adds support for the following datatypes:

  • Annotation URI
  • Geographic coordinates
  • Keyword
  • Monolingual text
  • Telephone number
  • Temperature
  • URL
  • Quantity
  • Code
  • Email

The following tables show the difference between master and this pull request. Note that most of the predefined/pre-deployed data types come with a predefined special page. (see the 3rd table) . This could be taken into account for future development to fix the differences between the 2 versions.

In few cases (e.g. "record", "keyword" and "reference") master performs better using the 'page' datatype. However, based on this data, the present PR use the same datatype in these cases as a temporary solution, so it performs better than master in any case. (except possible errors to be fixed).

MASTER

datatype user-defined label predefined label
Text
Code
Boolean
Number
Geographic coordinates
Temperature
Date
Email
URL
Annotation URI
Telephone number
Record ✓ (but wrong)
Quantity
Monolingual text ✓ (but wrong)
External identifier
Keyword
Reference

PULL REQUEST

datatype user-defined label predefined label
Text
Code
Boolean
Number
Geographic coordinates
Temperature
Date
Email
URL
Annotation URI
Telephone number
Record ✓ (using 'page')
Quantity
Monolingual text
External identifier
Keyword ✓ (using 'page')
Reference ✓ (using 'page')
datatype has predefined property page
Text yes
Code yes
Boolean yes
Number yes
Geographic coordinates yes
Temperature yes
Date yes
Email yes
URL yes
Annotation URI yes
Telephone number yes
Record no
Quantity yes
Monolingual text yes
External identifier yes
Keyword no
Reference no

@thomas-topway-it
Copy link
Contributor Author

the lint errors should now be fixed

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2023

Codecov Report

Attention: Patch coverage is 81.25000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 72.88%. Comparing base (781c713) to head (4499476).

Files with missing lines Patch % Lines
includes/Sql/SqlProvider.php 46.66% 8 Missing ⚠️
includes/Filter.php 88.23% 6 Missing ⚠️
includes/AppliedFilter.php 25.00% 3 Missing ⚠️
includes/Specials/BrowseData/GetCategories.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master      #62      +/-   ##
============================================
+ Coverage     72.54%   72.88%   +0.33%     
- Complexity      721      743      +22     
============================================
  Files            36       36              
  Lines          2302     2360      +58     
============================================
+ Hits           1670     1720      +50     
- Misses          632      640       +8     

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


🚨 Try these New Features:

@gesinn-it-wam
Copy link
Contributor

Have you considered writing (at least some) tests also for this PR?

@thomas-topway-it
Copy link
Contributor Author

I have also answered here #58

-- regarding the tests for this PR I think so far it is good they passed since this should reflect the fact that the changes did not change the previous functioning, but only add/cover some other cases. For these, yes, I could write more tests. The related test currently is "filters.json", so it could be added another set of filters for instance with name filters_extended.json

@thomas-topway-it
Copy link
Contributor Author

also, with reference of past PRs, this one #54 could be addressed quite quickly, I could double-check, while this one #48 seems quite complex ...

@thomas-topway-it
Copy link
Contributor Author

@gesinn-it-wam I have added the following test to capture the results of additional properties

4a4e839

I have still to complete it, I will post the complete test or another complementary test as soon as possible

@thomas-topway-it
Copy link
Contributor Author

added applied filter Knowledge-Wiki@8398d99

the test is now complete: all the new datatypes are captured except Monolingual Text (despite in the live site works). It could be related to the test config, not an error in the pull request

@thomas-topway-it
Copy link
Contributor Author

@gesinn-it-wam the error is Wikimedia\Rdbms\DBQueryError: Error 1146: Table 'wiki.smw_fpt_text' doesn't exist (mysql), I'm not sure what could be causing it, do you have any idea ?

@gesinn-it-wam
Copy link
Contributor

I think I have seen a similar error in the past... not sure if we were able to fix it. Please give me some time to investigate.

@krabina
Copy link
Contributor

krabina commented Nov 25, 2023

any news here?

@krabina
Copy link
Contributor

krabina commented May 3, 2024

This does not work for me. Tried https://test.knowledge.wiki/Spezial:Daten_durchsuchen/Cities when property size is set to number, filters are showing, when set to quantity, it displays "there are nor values for this filter".
Error! Unsupported property type (Maß) for filter Size.

@krabina
Copy link
Contributor

krabina commented Nov 21, 2024

@thomas-topway-it can you look into this?

@gesinn-it-gea
Copy link
Member

@thomas-topway-it can you please rebase to get the latest changes from the main branch. Now that testing is working again, we should make sure that changes are covered by testing. Ping @gesinn-it-ilm if you have any questions about tests and how to run them locally before committing.

@thomas-topway-it
Copy link
Contributor Author

unfortunately there are conflicts, I have to use the command line for that, I will check tomorrow

@thomas-topway-it
Copy link
Contributor Author

@gesinn-it-gea done (regarding tests, should I run explicitly tests locally ? ) @gesinn-it-ilm

@gesinn-it-gea
Copy link
Member

@thomas-topway-it after rebasing, you can run the tests locally. Simply run "make ci". If you need help ping @gesinn-it-ilm . Please check, that GF does not report any conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants