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

Sufia::FilesController versus CurationConcerns::FileSetsController #1424

Closed
awead opened this issue Dec 9, 2015 · 10 comments
Closed

Sufia::FilesController versus CurationConcerns::FileSetsController #1424

awead opened this issue Dec 9, 2015 · 10 comments
Assignees

Comments

@awead
Copy link
Contributor

awead commented Dec 9, 2015

With the change from Files to FileSets in the curation_concerns gem, what is the different between the Sufia::FilesController and the CurationConcerns::FileSetsController? Is the latter supposed to replace the former? If not, what's the distinction between the two?

@mjgiarlo
Copy link
Member

mjgiarlo commented Dec 9, 2015

Sufia's FilesControllerBehavior is Sufia-specific stuff that we inject into https://github.com/projecthydra/sufia/blob/master/app/controllers/curation_concerns/file_sets_controller.rb to make selective overrides and additions to the FileSetsController provided by CC. But that's not obvious by looking at the behavior in Sufia because there's no mention of CC in that file.

@awead
Copy link
Contributor Author

awead commented Dec 9, 2015

@mjgiarlo ok, that makes sense now, but isn't immediately obvious. Given the questions raised in #1427, I'm wondering if there's a way to make this clearer in the code. Renaming any instance of FilesController to FileSetsController might help, but also maybe some re-kajiggering of modules includes too?

@mjgiarlo
Copy link
Member

mjgiarlo commented Dec 9, 2015

Renaming would be a good start. Re-kajiggering is OK by me too if we can make the override a bit clearer.

@awead
Copy link
Contributor Author

awead commented Dec 9, 2015

My only thought on the re-kajiggering was moving the FileSetsController to app/controllers/file_sets_controller.rb so that:

class FileSetsController < ApplicationController
  include CurationConcerns::FileSetsController
  include Sufia::Controller
  include Sufia::FileSetsControllerBehavior
end

As it stands now, it's burried under app/controllers/curation_concerns

@mjgiarlo
Copy link
Member

mjgiarlo commented Dec 9, 2015

I thought it was in that directory intentionally, taking advantage of Ruby namespacing to effectively inject our concerns into CC's FileSetsController. There are other ways to do that of course.

@awead
Copy link
Contributor Author

awead commented Dec 9, 2015

Oh, ok, that makes sense then. I think I'm down with the further Files to FileSets renaming. I'll take that up. I think ideally, with also the Files versus Works distinction, there should be little occurrence of the word "file" or "files" in the final released gem.

@awead awead self-assigned this Dec 9, 2015
@awead awead removed the question label Dec 9, 2015
@mjgiarlo
Copy link
Member

mjgiarlo commented Dec 9, 2015

Agree that "file" in Sufia codebase should only be used in the pcdm:File context. OTOH, I think we should still label FileSets as "files" in the UI because our users have no idea what a FileSet is and (IMO) shouldn't need to. You probably meant this but I wanted to tease that out.

@awead
Copy link
Contributor Author

awead commented Dec 9, 2015

Yeah, good clarification!

@grosscol
Copy link
Contributor

@mjgiarlo for the UI, should the keys in the localizations be ui_file or something to that effect to explicitly delineate between where we mean "file as in pcdm::file" and "file as in the word the user will read" ?

@mjgiarlo
Copy link
Member

@grosscol I don't feel strongly about that at the moment. ¯_(ツ)_/¯ (but thanks for raising it, and PRs welcome from folks who feel more strongly about it than I do.)

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

No branches or pull requests

3 participants