Arvados

My first reactions after a tour:

• What "goes in folders"? Is there a definition for this currently, or should I be devising the definition?
• When copying a folder (currently there's only "Move" available), should the contents of the folder also be (recursively) copied?

Ah! Obviously! It's #goes_in_projects?!

Still wonder about the copying of contents of a folder.

Phil,
I had recently worked on move, copy, (and combine collections) for projects. The copy operation was only supported for non-folder type objects on a project. So, if a user selects one or more folders / projects, the copy action is disabled. Hope this helps.
--Radhika

Thanks Radhika. There's logic to that decision - certainly until such time as it is decided that copying a project's contents recursively is desirable, since the current behaviour isn't really sensible. My suggestion then would be to be consistent and centralize the decision somewhere. I thought about creating an argument for #goes_in_projects? called is_copy or something, and then having the logic be in there. Another even simpler idea would be a second ArvadosModel method called #copies_into_projects? which would by default return what #goes_in_projects? returns but which for the Group class would be overridden with false.

• Phil, I accomplished this using "workbench/app/assets/selection.js"
  

      $('[data-selection-action=copy]').
          closest('li').
          toggleClass('disabled',
                      ($checked.filter('[value*=-j7d0g-]').length > 0) ||
                      ($checked.length < 1));
  

Phil,

I asked Tom about copying projects on IRC, and he remarked, "if that happened it would be very pleasing but 3661 was meant to be more about filling in the missing buttons for which all of the back-end code is already done." So I think copying projects is out of scope for this story.

However, this isn't a policy decision, just a story scope one. As you've observed, it does make sense to talk about copying projects between each other, but the infrastructure for that is a little more complicated and we don't have it right now. Because of that, I think it might actually make sense to encode this case as a special exception for the copy button, rather than trying to centralize that information across the models. But I'm not 100% sure myself—if you think it makes sense to express this as a boolean method, I'd say go for it.

Your branch adds a goes_in_projects? method to the API server's ArvadosModel, but I'm not sure I understand what it's for—it doesn't seem to be used anywhere else. Can you please explain?

Everything else looks good. Thanks.

The goes_in_projects? was already present in every single model already inheriting from ArvadosModel that is supposed to go in projects. I also did notice at first (evidenced in my comments even here!) that this method existed. So my "class inheritence instinct" was to define a default in ArvadosModel for this method - to my mind this makes overriding it in all the other models more clear.

Similarly, I'm suggesting to add a copies_to_projects? at the ArvadosModel level and then override it for Group only. The reason that I think this makes sense is because the decision for whether something should be copy-able seems to be made in several places: Radhika has pointed out that in the copy/move selections logic copying is disabled if projects are included. Later once the infrastructure allows for copying projects we only need remove the Group#copies_to_projects? method. Tell you what, I'll just do it later this afternoon and we can always change it back if we don't like it.

Phil Hodgson wrote:

The goes_in_projects? was already present in every single model already inheriting from ArvadosModel that is supposed to go in projects. I also did notice at first (evidenced in my comments even here!) that this method existed. So my "class inheritence instinct" was to define a default in ArvadosModel for this method - to my mind this makes overriding it in all the other models more clear.

I think there's a little confusion between Arvados components here. You're right that Workbench models have this method—but that's true even for Workbench's base model, ArvadosBase (apps/workbench/app/models/arvados_base.rb). Your branch adds the method to the base class for API server models (services/api/app/models/arvados_model.rb), which are unrelated on a code level.

Similarly, I'm suggesting to add a copies_to_projects? at the ArvadosModel level and then override it for Group only. The reason that I think this makes sense is because the decision for whether something should be copy-able seems to be made in several places: Radhika has pointed out that in the copy/move selections logic copying is disabled if projects are included.

Sorry, I was slightly rushed this morning and didn't completely track the earlier conversation. That all makes sense to me, so just let me know when it's up and I'll take another look when it's ready. Thanks.

You're absolutely right about that confusion. What I did to ArvadosModel makes no sense and belongs in ArvadosBase, where in fact the method already exists. No wonder it worked so well. :)

Alright so I've committed the idea. If we agree then perhaps the #copies_to_projects? could be used in the selection-copying logic too, for disabling the copy button.

Reviewing 700fcdd3

Phil Hodgson wrote:

Alright so I've committed the idea. If we agree then perhaps the #copies_to_projects? could be used in the selection-copying logic too, for disabling the copy button.

Actually seeing it in front of me now, I really like this approach. Thanks. If you want to go ahead refactoring older code to use this as appropriate, that sounds great to me.

One small thing I noticed testing by hand: on pipeline template pages, there's now two rows of buttons; "Run this pipeline" appears above the copy/move buttons. It looks a little awkward. I'm guessing they're supposed to be in a line?

Opa! The existing code for selection copying that would need to refer to this new model method for us to stay DRY is hard-coded Javascript in the assets. The simplest way to change how this would decide whether to disable the copy button would require a more fundamental change to how we deal with Javascript assets: it would have to be processed as an erb. I just pushed a change to show how this could work, but I'm wary of the proposal because I'm suggesting doing something that, to pursue it to its logical conclusion, might require us to refactor quite a lot of Javascript to take model class methods into account during asset construction. Really what I committed is sort of half-way hard-coded solution that only takes into account the new method, and nothing else. I'm not sure how we feel about this at this stage, and it may be better to leave the hard-codings in the js file as they are. Or we proceed incrementally and accept the new change of this one Javascript file to being an .erb.

NB that the way Rails works when putting these assets together, it does not re-interpret the .erb unless there is a change to it. I.e. it doesn't suffice for there to be a change to code referenced by the .erb for the .erb to be re-interpreted. This can be a bit of a trip-up when testing this stuff.

For me the buttons are properly in a row, so I can't reproduce what you saw. But I think it would be a separate issue if the layout wasn't accommodating our buttons properly, since I'm merely adding the buttons to content_for :tab_line_buttons, as I believe to be correct.

Ah hm! My guess is that it's because it's rendered as a button instead of a Bootstrap-button-styled link. In spite of Bootstrap's best efforts, buttons and links don't seem to be as interchangeable as we may like. My guess is that none of the buttons are actually submitting forms, so perhaps they can just be substituted for links. It feels to me though like this should be a different Redmine though, and that this one is ready for merging if it still looks sensible.

To me, the image Brett attached looks broken enough that we need to fix it before merging.

1. Plan A: change the buttons to links. I think this should work: IIRC they all just open dialogs using remote=true, and that works fine with link_to.
2. Plan B: can we do something like "display:inline" to the offending form tag to avoid breaking the layout?

Okay, I'll look into it!

Well it was as simple as we'd hoped: in fact there is only one "show" page that had a button that should have been a link, and it was the one you'd found. Rails succeeded where HTML and Bootstrap failed!

Phil Hodgson wrote:

Well it was as simple as we'd hoped: in fact there is only one "show" page that had a button that should have been a link, and it was the one you'd found. Rails succeeded where HTML and Bootstrap failed!

I think that's a first for this week—but it's good news. 4ac75eb looks good to merge to me. Thanks.