CottageLabs / willow

Willow is an implementation of the Fedora/Samvera stack by Cottage Labs. It is built with Docker containers, which simplify development and deployment onto live services.
http://willow.cottagelabs.com/
6 stars 2 forks source link

Feature/branding #249

Closed nimphal closed 7 years ago

nimphal commented 7 years ago

This does a couple of things - it replaces the institution name in the English locale with an environment variable, introducing some interpolation along the way. This updates all visibility forms, the terms agreement, the deposit agreement, the header and a few internals.

It also creates the ability to define a path to a logo in the env file. If present, it will render the logo_pic partial. This is quite experimental, no promises that it will look good, but the ability is there.

You will need the following ENV vars in your .env

INSTITUTION_NAME= Cottage Labs # or something else if you'd like to test
INSTITUTION_NAME_FULL= Cottage Labs LLP # or something else if you'd like to test
INSTITUTION_LOGO="/path/to/file" # this will look passable with images whose width is longer than their height
martyn-w commented 7 years ago

Hi @Nimphal, I agree that the use of ENV variables throughout the application is not best practice; instead, consider making a new initializer file e.g. willow/config/initializers/organisation.rb and in that file, read the ENV variables (complete with defaults if not present) and store in some config variables. Then use the config variables as required throughout the application. See this guide for creating config variables: http://guides.rubyonrails.org/configuring.html#custom-configuration

Re internationalisation: I would de-scope that particular task for now, lets get the basic system working first.

Lastly there seems to be duplication going on between willow/app/services/institution.rb and willow/app/helpers/branding_helper.rb; could you refactor this?

nimphal commented 7 years ago

@anusharanganathan and @martyn-w yes, there is duplication between branding_helper and institution.rb. Institution.rb is used in the backend to populate the database amongst other things, so it needs to exist. It should, in theory, be enough for the interface as well. However, the permission labels take their content from some other helpers in Hyrax and those, regardless of my efforts, default to the Hyrax institution.rb as the source of the institution_name variable rather than the one defined in Willow. Probably because they are so deep in the hierarchy. The branding_helper overrides what the variable institution_name points to for the interface. It's definitely hacky, but after many hours trying to do it right, I accepted defeat.

I am taking a look at the config link posted above, but may not have to time to play with it this week.

martyn-w commented 7 years ago

Hi @Nimphal, if you don't mind (I don't want to tread on your toes), I can have a look tomorrow at removing some of the duplication? It may just require a couple of includes in the right places?

nimphal commented 7 years ago

I would be careful spending your time on this, @emanuil-tolev already banged his head against it for a few hours and ended up at the same conclusion. I am tempted to leave it as a hack (unless something reveals itself today) and make sure to comment it as such, so next time we do branding, hopefully with less of a rush, we know to weed it out.

martyn-w commented 7 years ago

Hi Nev, understood, how about 30 minutes to refactor and give up if it won't work?

nimphal commented 7 years ago

Hi both, I have moved things to configs (thanks Ryan!), figured out why the helper was necessary and subsequently removed the need for it. Things are much cleaner now and a lot less is actually changed from Hyrax originals. We also do the interpolation in only one place now.

martyn-w commented 7 years ago

Pull in the latest master branch - that will fix the broken tests, also change the ENV var in the html template to the Willow config var, then good to merge 👍