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

Clean Up TermController Logic #137

Open
no-reply opened this issue Feb 22, 2017 · 3 comments
Open

Clean Up TermController Logic #137

no-reply opened this issue Feb 22, 2017 · 3 comments
Labels
Milestone

Comments

@no-reply
Copy link

TermController has a bunch of logic for building authorities based on a largely undocumented namespacing pattern: https://github.com/projecthydra-labs/questioning_authority/blob/master/app/controllers/qa/terms_controller.rb#L35-L64

I haven't been able to completely make sense of everything that goes on here, a lot seems pinned to obscure specifics of various authority impls. We should replace the whole thing with a factory method an authority factory method and some kind of registry. I think it should be possible to accomplish this without breaking changes.

I have a branch that starts to work on this which fails a bunch of tests if you start to register authorities, various things break for reasons that are hard to pin down: https://github.com/projecthydra-labs/questioning_authority/tree/authority-factory

@no-reply
Copy link
Author

no-reply commented Feb 22, 2017

Part of the complication seems to be that various authorities don't bother to raise NotImplementedError for #all, but I'm not sure why returning them from a factory method, rather than constantizing, makes any difference.

Several authorities fail miserably when altered to inherit from Authority::Base.

@awead
Copy link
Contributor

awead commented Feb 22, 2017

@no-reply This was part of the original implementation where you could define your own authority as a constant like MyAuthority and the controller would register it for you by attempting to constantize it. Knowing what I know now, there are far better ways to do this. You're on the right track, I think, with your changes. init_authority should be removed from the controller completely and all it should do is consult the registry of authorities. If it's there proceed, if not, return a 501 or something.

I'm only vaguely familiar with the factory pattern, but if that can enforce the creation of new authorities, then I'm 👍 . The process of extending ::Base and raising "not implemented" errors was more a poor man's process. It does get you to write the methods you're supposed to, but there was never a documented process as to how to get to that point.

Feel free to ping me on any PRs or commits and I'll offer what feedback I can. Otherwise, I think any changes you're proposing regarding shoring up the API and codebase would be most welcome.

no-reply pushed a commit to curationexperts/qa-ldf that referenced this issue Feb 22, 2017
Authority registration is a hard coded constantize within this
namespace. Pending work on a registry, this aliases `Qa::LDF::Authority`
classes to the namespace to expose them to Questioning Authority.

See: samvera/questioning_authority#137
@mjgiarlo mjgiarlo modified the milestone: Backlog Mar 21, 2017
no-reply pushed a commit to curationexperts/mahonia that referenced this issue Jan 27, 2018
MESH ids are in lowercase. Since query is case sensitive, we need to downcase
query terms in order to prevent false negatives based on typist idiosyncracies.

Since it's not possible to redirect authorities in `Qa` due to the `TermsController`
behavior (samvera/questioning_authority#137), we
simply monkeypatch the existing `Qa::Authorities::Mesh` class.
no-reply pushed a commit to curationexperts/mahonia that referenced this issue Jan 27, 2018
MESH ids are in lowercase. Since query is case sensitive, we need to downcase
query terms in order to prevent false negatives based on typist idiosyncracies.

Since it's not possible to redirect authorities in `Qa` due to the `TermsController`
behavior (samvera/questioning_authority#137), we
simply monkeypatch the existing `Qa::Authorities::Mesh` class.
no-reply pushed a commit to curationexperts/mahonia that referenced this issue Jan 27, 2018
MESH ids are in lowercase. Since query is case sensitive, we need to downcase
query terms in order to prevent false negatives based on typist idiosyncracies.

Since it's not possible to redirect authorities in `Qa` due to the `TermsController`
behavior (samvera/questioning_authority#137), we
simply monkeypatch the existing `Qa::Authorities::Mesh` class.

Closes #244.
@elrayle elrayle added the wontfix label Mar 4, 2019
@elrayle
Copy link
Contributor

elrayle commented Mar 4, 2019

I am not expecting this to be fixed in the next release of QA. I am leaving open in case we want to revisit this later.

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

No branches or pull requests

4 participants