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

Prevent error when the system is configured to use dc:modified, but neither concept nor scheme have dates #840

Merged
merged 1 commit into from
Jan 28, 2019

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Jan 19, 2019

Following up on #839 , this pull request adds one more test case that should cover the case when the vocabulary is enabled to use the modified dates. But neither concept nor scheme have any dates.

Previous to this change, it would result in an unexpected error due to calling a method on a null value. Wrote a small unit test, and executed locally to confirm others worked, but this one was failing now.

Testing started at 6:39 PM ...
/usr/bin/php /home/kinow/Development/php/workspace/Skosmos/vendor/phpunit/phpunit/phpunit --bootstrap /home/kinow/Development/php/workspace/Skosmos/vendor/autoload.php --configuration /home/kinow/Development/php/workspace/Skosmos/phpunit.xml --filter "/(::testGetModifiedDate)( .*)?$/" WebControllertest /home/kinow/Development/php/workspace/Skosmos/tests/WebControllerTest.php --teamcity
PHPUnit 6.5.5 by Sebastian Bergmann and contributors.

Error : Call to a member function getValue() on null
 /home/kinow/Development/php/workspace/Skosmos/controller/WebController.php:264
 /home/kinow/Development/php/workspace/Skosmos/tests/WebControllerTest.php:116
 
Time: 1.93 seconds, Memory: 6.00MB

ERRORS!
Tests: 5, Assertions: 4, Errors: 1.

Generating code coverage report in Clover XML format ... done
Generating code coverage report in HTML format ... done
Process finished with exit code 2

Then patched the current code to check if the dc:modified value from the concept scheme exists. If not, it will simply return null as in the other case when there is no date for a concept, and everything should work fine (i.e. 200 HTTP responses).

Also added more comments to one method, and also to the unit test data provider, both for myself, and also for any future developer maintaining this feature 😁

@osma , would you be able to test this one against IPTC, please?

Cheers
Bruno

@osma
Copy link
Member

osma commented Jan 19, 2019

Thanks! Sure I can test, but not now. If you want to try it yourself you can copy the iptc config from here: https://github.com/NatLibFi/Finto-data/blob/master/conf/dev.finto.fi/config.ttl

The SPARQL endpoint is open.

Alternatively you can download IPTC or any other vocabulary from dev.finto.fi and load it into your local Fuseki.

@kinow
Copy link
Collaborator Author

kinow commented Jan 19, 2019

you can copy the iptc config from here

😮 For other vocabularies from Finto I would always go to its page, download the data, and manually craft the config! Thanks @osma !

@kinow
Copy link
Collaborator Author

kinow commented Jan 19, 2019

Now it got too easy to test Skosmos with the configuration from Finto @osma !

Using master, with Finto's config (i.e. not using the modified dates) loaded the IPTC config, then opened a concept, and then clicked on the concept again. All good.

screenshot from 2019-01-20 00-25-09

Toggled the setting for modified date, so that they would now be used. And got the same error you probably had before.

screenshot from 2019-01-20 00-25-45

Then did a git checkout - to switch back to the previous branch (prevent-error-when-304-no-dates from this pull request).

And looks like it did the job! No more error. Also no 304, but that's expected as the vocabulary is not using modified dates.

screenshot from 2019-01-20 00-26-39

Sorry about the bug 🐛 !

Cheers
Bruno

@osma osma merged commit ac35b9a into NatLibFi:master Jan 28, 2019
@osma
Copy link
Member

osma commented Jan 28, 2019

Seems to work, thanks a lot!

@osma osma added this to the 2.2 milestone Nov 29, 2019
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.

2 participants