3032 support favorites in path selector

[HazelGrant]

Fixes #3032

Screen.Recording.2023-11-15.at.1.59.16.PM.mov

[johrstrom]

Now that I see this, I think we should likely have a boolean to show favorites or not, but not allow for redefinition - at least not yet.

I'm trying to think of a use case where the user (the admin setting the app up OR the user interacting with it) would need an entry here that's not already in OodFilesApp#favorite_paths, but can't really think of one.

[johrstrom]

Thinking about this some more - I see some value in allowing for redefinition.

So, I think the interface could be something like this. Responding to false to disable it altogether.

attributes:
  path:
     widget: path_selector
     favorites: false

Then if users do want to override the favorites, they can supply an array of actual file paths. I think what you have now is json with hrefs - we can directly cast a local file path to a URL through files_path(path.to_s ,fs: 'fs').

Right now we'll limit it to local file paths, not supporting for remote file paths. Plus no support for new titles too. These could come later, if at all.

attributes:
  path:
     widget: path_selector
     favorites: 
       - /tmp
       - /fs/ess/scratch
       - /fs/ess/project

[HazelGrant]

Wouldn't this still require grabbing the filesPath in the JavaScript? If these paths don't come in with a filepath/file system, they don't work without it, and if you add fs: 'fs' to the files_path helper it would end up returning fs/tmp, right?

[johrstrom]

Wouldn't this still require grabbing the filesPath in the JavaScript?

I may have to look at it a little closer, because I do recall that we show/expose filesPath but need to refresh my mind as to why.

If these paths don't come in with a filepath/file system, they don't work without it,

Yea I think we should hard code/expect/support only local file systems. And this is by hard coding the fs: 'fs' argument.

it would end up returning fs/tmp, right?

files_path('/tmp', fs: 'fs')
would resolve to
/pun/sys/dashboard/files/fs/tmp (or the dev variant depending) which is the valid URL path.

The fs here in the URL indicates it's the local filesystem we're looking at. Any other value is a remote file system.

[johrstrom]

Wouldn't this still require grabbing the filesPath in the JavaScript?

I may have to look at it a little closer, because I do recall that we show/expose filesPath but need to refresh my mind as to why.

OK - I'm seeing how we could do it either way probably. Provide (to javascript) the file system location only OR provide the the URL path.

ondemand/apps/dashboard/app/javascript/path_selector/path_selector_data_table.js

Lines 159 to 160 in d291b21

	// note that this is storing the file system path, not the path of the URL
	// i.e., '/home/annie' not '/pun/sys/dashboard/files/fs/home/annie'

I guess my point is really about the configuration interface - not so much the implementation. I could go either way on implementation as it's clear we're transforming these back and forth.

But for the user creating this YAML, they shouldn't have to know or care about URLs - they should be able to just supply an array of local file system paths.

[johrstrom]

LGTM, thanks!

[HazelGrant]

LGTM, thanks!

I added one more commit to make it so that if favorites is false, the entire table won't show up. Just checking to see if you'd approve that as well.

[johrstrom]

LGTM, thanks!

I added one more commit to make it so that if favorites is false, the entire table >> won't show up. Just checking to see if you'd approve that as well.

I would, but it appears you need the default for fetch if there's no key. Tests fail with - ActionView::Template::Error: key not found: :favorites

Looks like you can use try -

irb(main):003:0> require 'rails'
=> true
irb(main):004:0> {}.try(:foo)
=> nil
irb(main):005:0> {}.fetch(:foo)
(irb):5:in `fetch': key not found: :foo (KeyError)
        from (irb):5:in `<main>'
        from /home/jeff/apps/ruby/3.0.6/lib/ruby/gems/3.0.0/gems/irb-1.3.5/exe/irb:11:in `<top (required)>'
        from /home/jeff/apps/ruby/3.0.6/bin/irb:23:in `load'
        from /home/jeff/apps/ruby/3.0.6/bin/irb:23:in `<main>'

[johrstrom]

I may have been mistaken about try on a hash - I don't actually know if it's able to pull anything from it. (I haven't pulled this down and tried interactively yet).

irb(main):019:0> f = {:foo => 'dfgdsf'}.try(:foo)
=> nil
irb(main):020:0> f = {foo: 'dfgdsf'}.try(:foo)
=> nil
irb(main):021:0> f = {foo: 'dfgdsf'}.fetch(:foo)
=> "dfgdsf"

[HazelGrant]

I may have been mistaken about try on a hash - I don't actually know if it's able to pull anything from it. (I haven't pulled this down and tried interactively yet).

irb(main):019:0> f = {:foo => 'dfgdsf'}.try(:foo)
=> nil
irb(main):020:0> f = {foo: 'dfgdsf'}.try(:foo)
=> nil
irb(main):021:0> f = {foo: 'dfgdsf'}.fetch(:foo)
=> "dfgdsf"

It has to be try(:[], :key)

The full functionality is working with this last commit.

[johrstrom]

Works for me! It doesn't exactly handle errors right (but doesn't crash! if i select something off the the allowlist it doesn't show the error, but also doesn't reset into some bad state), but we can pick that up later (and also in another ticket because favorites outside of the allowlist shouldn't show up anywhere).