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

[WIP] add externalFile support to Contents section #516

Closed
wants to merge 1 commit into from

Conversation

drh-stanford
Copy link

This PR fixes #485. It adds External File information into the Contents section of the catalog show page. The _default_contents.html.erb has ZERO tests, and all the data parsing, etc. is done in the view that is far from DRY. I did a major overhaul of the code formatting for the view to make it more readable hopefully. The only real part I added was the support for the externalFile elements. To test it, you need to create a contentMetadata datastream that has data as described in #485. If the manual testing is not acceptable, then I'd prefer to refactor how the view works (recommendations?). I also made some minor adjustments to the CSS styling.

Here's what the new display looks like:

screen shot 2016-02-12 at 12 17 25 pm

@mejackreed
Copy link
Contributor

Sigh... so your work looks great here. The sigh is for the view stuff already in there. A few thoughts on how to approach.

  • We really shouldn't be assigning variables in the view like this. This is an actual use case for moving logic to a helper.
  • There is a big opportunity to break this out into multiple partials. I could see something like _default_contents_resource.html.erb and _default_contents_file.html.erb. Each of those individual partials could be passed the locals it needs to render properly.
  • ContentMetadata resources could be broken out into their own classes. This would be useful for testing purposes and maintainability. sul-embed does something similar here with its Resources and ResourceFile classes generating objects based off of purlxml: https://github.com/sul-dlss/sul-embed/blob/master/app/models/embed/purl.rb#L138-L204

This is a tough one, not sure what the best way moving forward is on all of this or how much we can actually get done.

@drh-stanford
Copy link
Author

Because there's so much refactoring to be done, perhaps we should open a new technical debt issue for it? This PR just does some cleanup, no refactoring.

@mejackreed
Copy link
Contributor

I'm 👌 with that.

@drh-stanford
Copy link
Author

ok i created #536

@drh-stanford
Copy link
Author

@mejackreed is there anything else I should address in this PR?

@atz
Copy link
Contributor

atz commented Feb 19, 2016

can_manage, can_view, etc. really aren't defined anywhere else in the application? I swear I've seen the same logic before. Helper would be reasonable.

Side note: I prefer the long-line version of those assignments instead of the ugly paragraphs.

@dazza-codes
Copy link
Contributor

This PR will have conflicts with the APO PR because of the user permission calls like can_manage or can_view. So if this is go ahead, it's a blocker for the APO PR and that will have to be rebased on this.

Blocks #532

@drh-stanford drh-stanford changed the title add externalFile support to Contents section [WIP] add externalFile support to Contents section Feb 19, 2016
@drh-stanford
Copy link
Author

ok, i marked this WIP until #532 clears

@dazza-codes
Copy link
Contributor

The #532 changes that conflict with this can be seen clearly in ea1b53b

@drh-stanford
Copy link
Author

I will resubmit this later...

@drh-stanford drh-stanford deleted the externalfile-content branch February 22, 2016 22:57
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.

5 participants