[AT] - added remove site modal, controller, and tests #64
Conversation
|
remove site rebased, tests updated, and ready for functionality review @rickmanelius , pending that approval also ready for technical review. |
|
@andrew-c-tran I'd say despite much work the communication about what we're removing still needs a little work. I like the "Not removing your files" thing. That's good.
|
There was a problem hiding this comment.
Unless I've done something wrong, this doesn't work yet. I get the error "Could not remove site... path.replace is not a function" I'm on d4b59f6 and ran it with npm install and then npm start.
After I click "OK" on the path.replace complaint, it goes off into spinning "Working" land, with no result and no way to fix. And the "Remove site from dashboard" button remains active.
Please make sure this has error checking and can't do that.
There was a problem hiding this comment.
Stubbing out preparation. Working on review now...
Testing
Performed by Rick Manelius as of 2018-01-04
Hardware
I’m using a laptop with the following high level specifications:
- MacBook Pro: Retina 15-inch Mid 2015
- 2.2 Ghz Intel Core i7
- 16 GB 1600 MHz DDR3 RAM
- 250 GB Hard Drive
- macOS Sierra 10.13.2
Software Setup
- Xcode
- Go 1.9
- Docker 17.12.0-ce-rc4
- Git 2.13.6
Repositories Used
Preparation
- Run make clean && make darwin
- Verified I have macOS and DMG generated
- Run
electron .and app displayed - Installed WordPress 4.9.1 and Drupal 7.56 sites operational with an active database.
Results
Fails
- Deleting Data drud/ddev-ui/issues/19
- ...with an active Drupal 7.5.6 site showing up as a card in ddev-ui.
- I click on the card's dropdown menu represented by the third button with ...
- I see "Remove Site" as the 2nd of 3 items.
- I click "Remove Site" and see the following attachment (ddev-ui-pull-64-attachment-1-2018-01-04.png)
- Change Request: We're standardizing on "Project" over "Site" as per Standardize usage to "project", away from "app" or "site", at least in interacting with user ddev#526. We need to change "Remove Site" to "Remove Project".
- The UI's reliance on active Docker container labels to power
ddev listmakes it somewhat challenging to properly label what's happening. From the perspective of the end-user, removing the containers essentially removes it from the DDEV-UI dashboard, so the "Site from Dashboard" language seems accurate even though someone that bounces back and forth between the the CLI and UI would need to think a bit because they wouldn't necessarily make the connection between active containers as the prerequisite to populate the ddev-ui. To that end, would it be more appropriate to say "Project Containers" and "Project's Containers and Databases" as the two options. - I like the warning and think the "removal option" could be rewritten to "Please select an option:"
- I like the button updating to confirm the selection.
- It's important to highlight that this modal does not highlight WHICH project is being removed. I was busy writing up this review and left the modal active. I then forgot which site I was about to remove because it doesn't specify which project is about to be removed. I think we need to prepend in the modal's title. It could be something like "Remove $XYZ Project" where $XYZ is the project.
- I select "Site from Dashboard" and click the "Remove Site from Dashboard" button.
- I receive an electron error "Could Not Remove Site (TypeError: path.replace is not a function)" that I have to close.
- I then see the site removal starting with a spinner and the message "Removing..." appearing above the card with a knock out.
- I still see the "Remove Site from Dashboard" button persisting.
- I went to the command line and ran
ddev list. I see the site there and in the running state. - The ddev-ui app is stuck in the modal with the spinner
- This is not working at this time. I'll stop my review and return later.
Needs Testing
- Deleting Configuration drud/ddev-ui/issues/17
|
Idea. I'm not sure if this would work, but it might be helpful as part of these open PRs before testing for a quick screencast video by @andrew-c-tran to provide more of a tour of the functionality as working or as built. That 1-3 minute overview might help considerably given the time to test is considerable and it might help provide a faster turn on feedback. |
|
updated with better error state, as well as implemented verbiage and button changes. expanding test coverage now. can be reviewed for functionality during. |
|
@andrew-c-tran I'm having an issue where this is deleting the database regardless of which option is selected in the modal. I've traced this down to https://github.com/andrew-c-tran/ddev-ui/blob/16571c6dea008a0c08def83b9d0943caa12c7e28/js/remove-site.js#L19. Can you confirm that both values are passing through correctly? |
…ed updated test coverage
|
much more complete tests implemented, manual and automated testing confirm correct error states, handling, and removal flags being passed. |
|
Before my next round of manual testing, I'm going to take a stab at what an automated test might look like in terms of steps. I haven't reviewed the diff on this pull request prior to this, so this might already be in place or similar. Given that both the remove site and the remove database functionality require an active site running AND given that we already have the ability to generate a fresh site from a CMS distro, we can leverage what should be a pre-existing automated test routine of getting said site up and running before running through the tear down test.
Going to manual testing now... but happy to review this to sanity check whether this is too detailed of a test run or not detailed enough given where regressions could be introduced. |
|
Actually, I missed a step in my own test instructions. By default the site(s) won't have a populated database because we do not assume that. We can pass a drush site install command through drush exec that would provide that. UPDATE: Here would be a way to achieve the install via drush leveraging some of the work already done in the ddev codebase. https://github.com/drud/ddev/blob/master/testing/ddev-perf-test.sh#L80 |
|
I'll create a more thorough review when I get to the office, but I was able to confirm the two different options now work in terms of whether or not the db is removed or not. |
There was a problem hiding this comment.
This is approved from an end-user functionality perspective with one comment and two conditions.
- Comment: This PR purports to address issues Delete config #17 and Delete data #19. Unfortunately, the intention of Delete config #17 is to remove the configuration files (specifically ~/.ddev/config.yml and ~/.ddev/docker-compose.yml). This of course pre-supposses that we have removed the project containers so they are not left unattached. I'll update this in Delete config #17.
- Condition: We still need to address all the sudo, security, /etc/host considerations which has been a pain in the arse. There are multiple tickets to draw from and we'll do this in v0.3.
- Condition: We'll address the s/Site/Project/ changes in replace instances of "site"/"app" with "project" #73 when we're in v0.3 and clarifying our wireframes/design.
From my perspective, we just need a code/test review before merging.
|
Going to add @cweagans to assist on the code/test review and then we can get this merged. Thanks! |
There was a problem hiding this comment.
Site/Project terminology is still not consistent in the removal dialog, also still Add/Create "site". Sorry about being ***l about this, just thought that ddev-ui had pioneered the (excellent) "project" terminology.
(Edited to remove site card size issue which didn't seem to exist after a make clean)
Most of the functionality seems to be there now. I haven't looked at the code yet.
…ed code references for remove module to use project vs site. added name of project being removed to modal
…ui into issue_19_remove_site
On that last point, I was thinking of adding a "for more information on what these options do, please refer to the |
|
I just ran through this and the functionality of the remote-project seems fine now. I like the verbiage on the dialog box, thanks. I'll look at the code now. |
rfay
dismissed
their stale review
I've used this in its current state and it seems to be working fine and has the verbiage we want. Now I'll take a look at the code.
js/ddev-shell.js
Outdated
| const remove = (path, callback, errorCallback) => { | ||
| ddevShell('remove', null, path, callback, errorCallback, true); | ||
| const remove = (path, db) => { | ||
| var args = db ? ['-j', '--remove-data'] : ['-j']; |
There was a problem hiding this comment.
So the header isn't right at all here is it? db seems to be a bool that if set, does a remove? If so, could we rename db to "remove_data" or something? And aren't there only the 2 params?
There was a problem hiding this comment.
ah, i did miss the header on that update. i've updated it, and changed the ambiguous "db" variable to "shouldRemoveData" to be more descriptive
|
|
||
| return ddevShell.remove(removePath, removeData) | ||
| .then(function(){ | ||
| removeCompleted('Project Successfully Removed.'); |
There was a problem hiding this comment.
No big deal, but all caps on a sentence isn't all that normal. "Project successfully removed." would be OK. We probably need a style guide on this kind of thing, but all caps (except for NY Times headlines) isn't all that useful.
There was a problem hiding this comment.
good point, we do need to get consistent across the application. I'll leave this as is, and have opened issue #74 to get all messaging across the app consistent as there are many, many dialogs and prompts that we'll need to address, as well as a review of current verbiage being used.
js/remove-project.js
Outdated
| removeCompleted('Could Not Remove Project ('+err+')'); | ||
| }); | ||
| } catch(e) { | ||
| removeCompleted('Invalid Input Passed To Remove'); |
There was a problem hiding this comment.
You can just provide the exception as a string here like above right, and maybe quote the invalid input? Because if we ever have a customer hit this we'll sure want it.
There was a problem hiding this comment.
Sure, the formdata is an object, so the best way to display that would be to convert it to a json string and include that along with the actual error text.
js/site-cards.js
Outdated
| @@ -55,6 +55,7 @@ function createCard(site) { | |||
| </button> | |||
| <div class="dropdown-menu" aria-labelledby="dropdownMenuButton"> | |||
| <a class="dropdown-item restartbtn" href="#">Restart</a> | |||
| <a class="dropdown-item removebtn" href="#" data-project-name="`+site.name+`" data-project-path="` + site.approot + `">Remove Project</a> | |||
There was a problem hiding this comment.
You don't actually need the data-project-path to use ddev remove, and it really would generally be better IMO to use ddev remove <project> instead of going to the directory and issuing the command. It just seems simpler to me.
There was a problem hiding this comment.
So, originally in the prototypes when I was first writing the ddev-shell wrapper, I was indeed just calling remove with a site name. However, I began to notice that the basic functionality commands were not consistent. Some made sense, i.e. why we cannot start a site by name as we have no idea where it is, but why for instance can we stop a running site by name, but not restart a running site by name? So we were left with the fact that we can stop, remove, or describe a site by name, but must be in the directory to config, start, or restart one. In an attempt at consistency, I made it so every action takes a path. That way, every action you want to perform needs to know just the path, vs having to decide when to pass in name and when to use path.
It's pretty convenient from a development side to not have to think about that, to just know that every command will work if you give it a path, but if you feel strongly about it we can segment out which calls need a name and pass that, vs which ones need a path and pass that instead.
There was a problem hiding this comment.
Now I remember that whole thing. And I certainly agree with you that we could wish for consistency from ddev. Your push can help us with that though. Glad we've discovered that ddev can be improved to help ddev-ui work easier with easier code.
js/site-cards.js
Outdated
| @@ -55,6 +55,7 @@ function createCard(site) { | |||
| </button> | |||
| <div class="dropdown-menu" aria-labelledby="dropdownMenuButton"> | |||
| <a class="dropdown-item restartbtn" href="#">Restart</a> | |||
| <a class="dropdown-item removebtn" href="#" data-project-name="`+site.name+`" data-project-path="` + site.approot + `">Remove Project</a> | |||
| <a class="dropdown-item" onclick='electron.shell.showItemInFolder("` + site.approot + `")' href="#">Browse Local Files</a> | |||
There was a problem hiding this comment.
I thought the onclick stuff was old fashioned and not something people did any more.
There was a problem hiding this comment.
you'd be surprised, it seemed to fall out of favor for a while, but is now a pretty standard way of triggering click events in React-land. In this case though, it is inconsistent with the rest of the action bindings, and i'll move it out of there.
|
@rickmanelius can we get a final look at this before i merge it in? |
|
@andrew-c-tran I missed this notification until right now. Updating assignment and I'll get this by COB today. |
rickmanelius
requested review from
rickmanelius
and removed request for
cweagans
The Problem/Issue/Bug:
DDEV UI did not have the ability to remove a site
How this PR Solves The Problem:
remove site functionality with prompts and tests added
Manual Testing Instructions:
make build darwin*note: if you remove all your sites and the UI no longer has any sites (i.e. ddev list returns "no ddev apps running" message, the UI will error out. This has been corrected in Issue 14 add from cms #62 )
Automated Testing Overview:
DDEV Remove functionality tests written, testing calling ddev cli with and without --remove-data flag, and exiting with a 0 and non-0 code.
Related Issue Link(s):
#19 #17