Story #3661

[Workbench] Add "Move" and "Copy" buttons to top of #show page for every kind of object that goes_in_folders

Added by Tom Clegg about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Phil Hodgson
Category:
Workbench
Target version:
Start date:
09/10/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

pt-copy-move-2rows.png (8.11 KB) pt-copy-move-2rows.png Two rows of buttons on pipeline template page Brett Smith, 09/17/2014 11:49 AM

Subtasks

Task #3845: Add the buttons and make them workResolvedPhil Hodgson

Task #3851: Add one or two new integration tests for the new buttonsResolvedPhil Hodgson

Task #3852: Review 3661-copy-move-from-showResolvedPhil Hodgson

Associated revisions

Revision 67182ab9
Added by Phil Hodgson almost 5 years ago

Merge branch '3661-copy-move-from-show' refs #3661

History

#1 Updated by Ward Vandewege almost 5 years ago

  • Target version changed from Arvados Future Sprints to 2014-09-17 sprint

#2 Updated by Brett Smith almost 5 years ago

  • Assigned To set to Brett Smith

#3 Updated by Brett Smith almost 5 years ago

  • Assigned To deleted (Brett Smith)

#4 Updated by Tom Clegg almost 5 years ago

  • Category set to Workbench
  • Assigned To set to Phil Hodgson

#5 Updated by Phil Hodgson almost 5 years ago

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?

#6 Updated by Phil Hodgson almost 5 years ago

Ah! Obviously! It's #goes_in_projects?!

Still wonder about the copying of contents of a folder.

#7 Updated by Radhika Chippada almost 5 years ago

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

#8 Updated by Phil Hodgson almost 5 years ago

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));
    

#9 Updated by Brett Smith almost 5 years ago

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.

#10 Updated by Phil Hodgson almost 5 years ago

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.

#11 Updated by Brett Smith almost 5 years ago

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.

#12 Updated by Phil Hodgson almost 5 years ago

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. :)

#13 Updated by Phil Hodgson almost 5 years ago

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.

#14 Updated by Brett Smith almost 5 years ago

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?

#15 Updated by Phil Hodgson almost 5 years ago

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.

#16 Updated by Phil Hodgson almost 5 years ago

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.

#17 Updated by Brett Smith almost 5 years ago

Phil Hodgson wrote:

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.

I see what you mean, and I think I'd be happy with either way you decide. (Tom might have stronger opinions.) We tend to be pretty big on a philosophy of "lay the groundwork now, refactor later" around here, so I don't see anything wrong with merging the JavaScript in either state.

I've attached a screenshot of the button issue. This is Workbench pointed at a development API server with the test fixtures loaded, specifically /pipeline_templates/zzzzz-p5p6p-aox0k0ofxrystgw. Looking at the HTML, it looks like the Run button is wrapped in a form+div, so the browser has to render the other buttons under that block. I know that's nothing you did in your branch, but I do feel like it's important to smooth over this interaction one way or another in order to achieve the desired layout.

#18 Updated by Phil Hodgson almost 5 years ago

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.

#19 Updated by Phil Hodgson almost 5 years ago

  • Status changed from New to In Progress

#20 Updated by Tom Clegg almost 5 years ago

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

I haven't reproduced it myself, but if I'm following correctly I'd say
  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?

#21 Updated by Phil Hodgson almost 5 years ago

Okay, I'll look into it!

#22 Updated by Ward Vandewege almost 5 years ago

  • Target version changed from 2014-09-17 sprint to 2014-10-08 sprint

#23 Updated by Phil Hodgson almost 5 years ago

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!

#24 Updated by Brett Smith almost 5 years ago

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.

#25 Updated by Phil Hodgson almost 5 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF