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

Error on deleting an ontology that has provisionalClasses #127

Closed
Tracked by #212
syphax-bouazzouni opened this issue Feb 4, 2022 · 3 comments · Fixed by #128
Closed
Tracked by #212

Error on deleting an ontology that has provisionalClasses #127

syphax-bouazzouni opened this issue Feb 4, 2022 · 3 comments · Fixed by #128

Comments

@syphax-bouazzouni
Copy link

At Agroportal, when we try to delete an ontology that has provisionalClasses, we get this exception :

NoMethodError: undefined method `select!' for #<LinkedData::Models::Ontology:0x0000000004896968>
from /srv/ontoportal/ncbo_cron_deployments/shared/bundle/ruby/2.6.0/bundler/gems/ontologies_linked_data-db2377e860d7/lib/ontologies_linked_data/models/ontology.rb:367:in `block in delete'

And i think that the cause of the problem is here : https://github.com/ncbo/ontologies_linked_data/blob/master/lib/ontologies_linked_data/models/ontology.rb#L366

@jonquet
Copy link

jonquet commented Feb 4, 2022

After discussing this with @syphax-bouazzouni we see a discrepancy between what is in the model and what is in the delete ontology script.
The first one assumes a provisional class has one and only one ontology (see https://github.com/ncbo/ontologies_linked_data/blob/master/lib/ontologies_linked_data/models/provisional_class.rb#L16)
The second one assumes a provisionnal class can be in several ontologies (thus the onts.select!)

We believe the model is right and the script is wrong.
@mdorf @jvendetti Do you confirm ?

In that case, the onts.select! will be removed and changed by a couple of lines of code that will delete all the provisional classes when the ontology containing them is itself deleted.

We can PR the change if confirmed by you.

@mdorf
Copy link
Member

mdorf commented Feb 4, 2022

@jonquet, @syphax-bouazzouni, you are correct! This does appear to be a discrepancy. I think the code should be revised to:

self.bring(:provisionalClasses)
unless self.provisionalClasses.nil?
  self.provisionalClasses.each do |p|
    p.delete
  end
end

My concern is that our unit test suite did not catch this error. We should fix the corresponding unit test along with this change by adding a provisional class to one (or both) of the two ontologies created in the test:

https://github.com/ncbo/ontologies_linked_data/blob/master/test/models/test_ontology.rb#L298
https://github.com/ncbo/ontologies_linked_data/blob/master/test/models/test_ontology.rb#L299

Something like this on Line 303 should do the trick, I think (please verify):

pc = LinkedData::Models::ProvisionalClass.new({label: "Test Provisional Class", creator: u, ontology: o1})
pc.save

Good catch!

@jonquet
Copy link

jonquet commented Feb 10, 2022

@mdorf
We have fixed (@syphax-bouazzouni ) the script. Be sure to "update/control" the tests.

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 a pull request may close this issue.

3 participants