search

sul-dlss / argo

The administrative discovery interface for Stanford's Digital Object Registry
Other
20 stars 5 forks source link

Detail display, don't die on bad APO or Collection reference #334

Closed LynnMcRae closed 8 years ago

LynnMcRae commented 8 years ago

Objects shouldn't exist with a bad APO or collection reference, but they do, esp with test data. And this is exactly what the UI should show us should it actually happened for real, rather than getting a system error. In the case of these lookups, show the druid in place of an unresolved lookup, perhaps highlighted somehow (in red?) to signal the problem?

mejackreed commented 8 years ago

Examples?

LynnMcRae commented 8 years ago

sorry, this is one of the cases obscured by the generic "Incomplete response received from application". Apparently my bug report on this condition revealed both a date-interpretation issue and this issue. DarrenW may have an example ... looking in slack

drh-stanford commented 8 years ago

There's an example in https://github.com/sul-dlss/argo/issues/293 of the error message, but not an example druid

drh-stanford commented 8 years ago

To reproduce for an invalid APO:

The problem here is that any indexing of the object will now fail because the to_solr function no longer works for the object.

screen shot 2015-12-18 at 2 18 42 pm

So, I believe this fix is substantial as it will involve changing dor-services such that it can handle object not found errors in the (potentially many) to_solr functions.

Another possible fix would be to make the RELS-EXT edit window something that validates that correctness of the XML by checking that the APO and Collection druids are valid. This would only affect hand-edited cases, but those are arguably the most common source of invalid druids.

The workaround is to edit the RELS-EXT datastream directly from Fedora, which is cumbersome but would work ok.

LynnMcRae commented 8 years ago

If the to_solr method fails, then we won't be able to index objects in this state at all? This is a more serious situation than being able to find but not display them. No object should be absent from the Argo index altogether. Do we need to have to_solr catch the error and pass the druid along instead of the expected title in the cases of collection and APO lookups that can't locate the target object?

drh-stanford commented 8 years ago

@jmartin-sul to_solr fails in some cases, like this one where we get ObjectNotFound errors. the reindexing code doesn't handle this case, right? If so, we need to figure out what to do in the case of errors as we won't have the solr_doc at the reindex-call level (I marked this issue designNeeded for this reason). See the procedure above for how to get an object into this incorrect state. (Also, might this account for the Solr index being short some objects?)

screen shot 2015-12-18 at 2 15 09 pm

drh-stanford commented 8 years ago

btw, I meant RELS-EXT not identityMetadata in my examples above so I revised them with some screenshots.

blalbrit commented 8 years ago

@mejackreed - we're triaging tickets. Is this one still active, or - if we have an example to check - is it testable in dev or stage?

mejackreed commented 8 years ago

I'm not sure if this is still valid or not. Needs further investigation

drh-stanford commented 8 years ago

it's still an open problem. to reproduce it in -dev or -test, you can use the steps in my Dec 18th comment in this issue (basically hack the item to have a bogus APO).

dazza-codes commented 8 years ago

This might have been auto-closed by resolving a PR that tagged it with fix #334 but there are additional requirements to satisfy for this issue.

mejackreed commented 8 years ago

@darrenleeweber Could you list those requirements so they can be addressed?

dazza-codes commented 8 years ago

The main thing is that it looks like @drh-stanford is working on some aspect of this issue (from the commentary above) and #405 is still open.

Also, I want to understand how this dovetails with the APO PR. My concern is that all this work has been done without any of the logic in the APO PR at https://github.com/sul-dlss/argo/blob/issue76-apo-nice/app/models/user.rb#L166-L243, where it's possible this issue is already resolved. My primary point of evaluation is in the apparent conflict of specs in: https://github.com/sul-dlss/argo/blob/issue76-apo-nice/spec/models/user_spec.rb#L205-L211 vs. https://github.com/sul-dlss/argo/blob/issue76-apo-nice/spec/models/user_spec.rb#L238-L240

dazza-codes commented 8 years ago

I've now resolved my concerns about conflicting specs in commits today on https://github.com/sul-dlss/argo/pull/532

If we get that merged and deployed, the QA should sign off on this issue.

atz commented 8 years ago

Consensus in the room here is that this is accomplished within the scope of this ticket (and w/ corresponding fix in dor-services). Closing.