curtwagner1984 / YAPO

Yapo - Yet Another Porn Organizer
GNU General Public License v3.0
49 stars 10 forks source link

[Feature Request] Add confirmation for removing/deleting items from database or disk #31

Closed sjclayton closed 2 months ago

sjclayton commented 8 years ago

Needs to be some sort of confirmation before deleting items (scenes, actors, tags, etc.) from the database or in the instance of removing a scenes files from disk. The potential for accidentally clicking those buttons (remove/delete) is fairly high.

curtwagner1984 commented 8 years ago

@sjclayton You can try to do it yourself :)

First of all there is a 'confirm' function in JS: http://www.w3schools.com/jsref/met_win_confirm.asp

It returns True if the user press 'Ok' and False if the user presses 'Cancel'

So basically what you want is if the user wants to delete something, you summon this popup and if he presses 'Ok' you go on and do what Yapo already does now, if not, you do nothing.

Let's see an example:

<span ng-click="$ctrl.removeItem(scene,actor,'delete',false)"
                          class="glyphicon glyphicon-trash on-hover-red-cursor-pointer"
                          uib-popover-html="'<strong>Delete</strong> scene from database!'"
                          popover-trigger="mouseenter">
</span>

This is in scene-list.template.html and it's the html markup for the button that removes a scene from the database. We can identify it as such because that is what it says in the pop over:

uib-popover-html="'<strong>Delete</strong> scene from database!'"

The function that is invoked when this button is clicked is:

$ctrl.removeItem(scene,actor,'delete',false)

This function is inside scene-list.component:

self.removeItem = function (scene, itemToRemove, typeOfItemToRemove, permDelete) {
// function contents doesn't matter.
};

What we want is to create our own confirmation function, and if the users presses 'OK' we will execute the original function $ctrl.removeItem(scene,actor,'delete',false).

So let's write our confirmation function:


self.confirmationFunction = function(originalScene, originalItemToRemove, originalTypeOfItemToRemove, originalPermDelete){
     if(confirm('Are you sure you want to remove' + originalScene.name + 'from db ?' )){
        // we are here if the user pressed OK
       // Let's call the original function $ctrl.removeItem(scene,actor,'delete',false)
      self.removeItem(originalScene,originalItemToRemove,originalTypeOfItemToRemove,originalPermDelete)
     }else{
         // we are here if the user pressed cancel
        // we can do nothing in which case the else clause is useless or pop up an alert
        alert("Think more carefully next time!");
     }
}

self.removeItem = function (scene, itemToRemove, typeOfItemToRemove, permDelete) {
// function contents doesn't matter.
};

Now let's replace the function in the markup with our confirmation markup:

<span ng-click="$ctrl.confirmationFunction(scene,actor,'delete',false)"
                          class="glyphicon glyphicon-trash on-hover-red-cursor-pointer"
                          uib-popover-html="'<strong>Delete</strong> scene from database!'"
                          popover-trigger="mouseenter">

</span>

Notice that we want to pass the same arguments to our function so they can be later passed down to the removal functions.

P.S I you do try and make this yourself (And I would really appreciate it if you did) make sure that your confirmation function covers all the confirmation options for scenes. In other words so you won't use 2 diffrent confirmation functions for deleting a scene from db and deleting it from the hard-disk.

P.S.S In the html markup $ctrl means that the function or the variable is in the controller for that page. In this example the controller is the code on scene-list.component.js So anything that is created in the controller using self is accessible in the markup using '$ctrl' that is why we create our function using self.confirmationFunction = function () instead of var confirmationFunction = function ()

P.S.S.S I realize that a point could be made that in the time it took me to write this I could have made those changes myself, but hopefully this will give you and anyone else who reads this an insight into Yapo's inner workings so you can change other things and not just this function.

sjclayton commented 8 years ago

@curtwagner1984 I am going to take a stab at this... just working right now on getting my development environment set up on a separate machine =D

On that machine I am going to clone my fork of YAPO from my repository, and start to work on it more in depth.

Things I am planning to work on in the near future include... a major rework of YAPO's CSS styling, scrapers for IAFD and Boobpedia, and other goodies!

curtwagner1984 commented 8 years ago

Awesome ! BTW, I'm using Pycharm as an IDE. It can integrate with github and can be configured to work with virtual environments. And as far as I know is platform independent.

sjclayton commented 8 years ago

@curtwagner1984 Did you buy the Pro Version?

curtwagner1984 commented 8 years ago

@sjclayton Nah... So far I just use the 30 day trail of the pro version. I'll think what to do when it ends.

sjclayton commented 8 years ago

I am going to push a commit shortly.... hopefully you're still around.

sjclayton commented 8 years ago

@curtwagner1984 Right now it only works with removing a scene from the database, and not the disk... as I can't seem to figure out how to disable the code that makes it so that the function to delete from disk only works when in multiple tag/delete mode when at least one item is selected.

EDIT: Also -- I just bought the Pro version of PyCharm! =D I got sick of trying to get Eclipse set up with all the plugins I wanted and getting them all to work properly with each other... PyCharm appears to 'just work' and have everything I could want / need.

curtwagner1984 commented 8 years ago

@sjclayton Yeah, Pycharm is awesome!

What do you mean

" I can't seem to figure out how to disable the code that makes it so that the function to delete from disk only works when in multiple tag/delete mode when at least one item is selected."

Can't you just use the same code and wrap it with the confirmation function code ?

sjclayton commented 8 years ago

@curtwagner1984 Ok, maybe I don't get which code you're referring to, or how you're referring to it.... =P (Maybe, I'm slightly slow... LOL)

This is the code in actor-detail.template.html right now:

                   <span ng-click="$ctrl.confirmRemove(scene,actor,'delete',false)"
                          class="glyphicon glyphicon-trash on-hover-red-cursor-pointer"
                          uib-popover-html="'<strong>Delete</strong> scene from database!'"
                          popover-trigger="mouseenter">

                            </span>

                    <span ng-if="$ctrl.tagMultiple" ng-click="$ctrl.removeItem(scene,actor,'delete',true)"
                          class="glyphicon glyphicon-fire on-hover-red-cursor-pointer"
                          uib-popover-html="'<h2><font color=&quot;red&quot;>CAUTION!</h2></font><strong>Will Delete
                          files on disk</strong> without asking for confirmation! <i> Will only work if at least one
                          item is checked this is to avoid accidental deletion.'"
                          popover-trigger="mouseenter">

                    </span>

but if I change it to something like this (removing the ng-if, which is what I assume is telling it to require tag multiple items to be selected):

                   <span ng-click="$ctrl.confirmRemove(scene,actor,'delete',false)"
                          class="glyphicon glyphicon-trash on-hover-red-cursor-pointer"
                          uib-popover-html="'<strong>Delete</strong> scene from database!'"
                          popover-trigger="mouseenter">

                            </span>

                    <span ng-click="$ctrl.confirmRemove(scene,actor,'delete',true)"
                          class="glyphicon glyphicon-fire on-hover-red-cursor-pointer"
                          uib-popover-html="'<strong>Delete</strong> scene from disk!'"
                          popover-trigger="mouseenter">

                    </span>

It doesn't work... it still appears to be somehow expecting to have to have the tag multiple items, and have atleast one item checked..

Basically when I tried implementing it like that, it popped the dialog asking for confirmation up... but when I clicked on 'OK' it just dropped right back to the page and didn't actually execute the function. It should be passing (scene,actor,'delete',false) when one button is clicked and (scene,actor,'delete',true) when the other is clicked to my confirmRemove function and then on down to the removeItem function.... should it not..? the true / false part is what ends up telling it whether or not it's just removing from the DB or if it's removing from the disk, correct?

curtwagner1984 commented 8 years ago

@sjclayton

true / false part is what ends up telling it whether or not it's just removing from the DB or if it's removing from the disk, correct?

Correct!

I mean why didn't you do it with the ng-if?

Instead of this:

<span ng-if="$ctrl.tagMultiple" ng-click="$ctrl.removeItem(scene,actor,'delete',true)"
                          class="glyphicon glyphicon-fire on-hover-red-cursor-pointer"
                          uib-popover-html="'<h2><font color=&quot;red&quot;>CAUTION!</h2></font><strong>Will Delete
                          files on disk</strong> without asking for confirmation! <i> Will only work if at least one
                          item is checked this is to avoid accidental deletion.'"
                          popover-trigger="mouseenter">

                    </span>

Make it like this:

<span ng-if="$ctrl.tagMultiple" ng-click=$ctrl.confirmRemove(scene,actor,'delete',true)
                          class="glyphicon glyphicon-fire on-hover-red-cursor-pointer"
                          uib-popover-html="'<h2><font color=&quot;red&quot;>CAUTION!</h2></font><strong>Will Delete
                          files on disk</strong> without asking for confirmation! <i> Will only work if at least one
                          item is checked this is to avoid accidental deletion.'"
                          popover-trigger="mouseenter">

                    </span>

And in confirmRemove:

self.confirmRemove = function (originalScene, originalItemToRemove, originalTypeOfItemToRemove, originalPermDelete) {

                var message = "";

                if (originalPermDelete){    //If delete from disk
                           message = "Are you sure you want to delete selected scenes from DISK?! Are you crazy? Think about all the bandwidth it took to acquire it ...";
                 }else{
                          message = 'Are you sure you want to remove ' + originalScene.name + " from the DB?";
                 }
                if (confirm(message)){
                    self.removeItem(originalScene,originalItemToRemove,originalTypeOfItemToRemove,originalPermDelete);
                } else {
                    // if in else you do nothing, it can be omitted.
                }

            };
sjclayton commented 8 years ago

@curtwagner1984 I don't know why I didn't do that.... I tried something similar to that and it still didn't work... mainly I had something slightly different for the if (originalPermDelete){ part.... I will get it!! =P.... you're explaining things quite well... eventually once I refresh my brain, I won't need as much help...

Question.... If I do change it to be like that.... is it still going to then require 'Tag / Delete Multiple Items' to be selected and have at least one item checked...? (I'd assume probably...)

I mean, do you still want it to behave like that...? as it's kind of redundant to have basically two forms of preventing accidental deletion of a file... as requiring an item to be checked is sort of a confirmation already in a way.

curtwagner1984 commented 8 years ago

@sjclayton Yes it will still require at least one scene to be selected. As far as I remember the requirement for selecting at least one element is a "bugfeature" a "febug" a "bugture".

The infrastructure of handling selected items was already in place (from the remove from db functions), so it was much easier to implement deletion of multiple items using the same function than creating a new function that handles individual deletion.

So I told myself, "it's easier to implement AND it adds another layer of protections from deleting a file by mistake" Win-Win scenario!

Image of Yaktocat

Probably should fix that later on though ...

thierry400 commented 8 years ago

Is this option supposed to be working already? Since I just noticed that I can't delete any scene's out of the database.

I know that it used to work (maybe a week or so ago?) but now when i try to remove from db + delete from pc It just does nothing at all, and If I only try to remove from db I get a popup to confirm I want to delete the scene from the database. If I agree on the popup the scene also nothing happens (the scene doesn't get removed from the database)

sjclayton commented 8 years ago

@thierry400 Looking into it now.... both functions should be working without issue... but I just confirmed the 'remove from db' function isn't working on my end now either, and it was just the other day.... I don't think anything got changed recently having to do with that part of the code...

But like I said I'll take a look..

sjclayton commented 8 years ago

@thierry400 Apparently got changed that is affecting it (I did noticed some changes in the files that handle it when looking at the commits, that might be part of the problem) -- the changes for this issue itself have not yet been fully implemented... but that still shouldn't be causing any issues with the way it's supposed to work with the way the code currently is...

@curtwagner1984 will have to look at it... as something that got changed in the last few days is breaking the basic functionality.

curtwagner1984 commented 8 years ago

@thierry400 @sjclayton Remove from database doesn't work, but deleting files works ? Also what the console(and chrome devtools) are saying when you trying to delete?

curtwagner1984 commented 8 years ago

Should work now, yet still needs testing

sjclayton commented 8 years ago

@curtwagner1984 Your last changes appear to have it working again now (the deletion functions), I just gave it a test... I am going to finish on the work for the confirmation dialogs later for now atleast.... (because they might still need to be / should be reworked down the road, as the JS alert function looks like shit - was thinking about looking more into Angular's dialog / popups either that or maybe jQuery even).

curtwagner1984 commented 8 years ago

@sjclayton Well it should be done with a modal. (Like the one that is used when adding an image) https://angular-ui.github.io/bootstrap/ You can play around with the modal options, and look at how it's done for the actor image in actor details. It takes considerably more work than using js built in confirm which does look like crap

thierry400 commented 8 years ago

I've ben testing the delete options today and I noticed that it is now working for just removing files from the database. Permenantly removing videos however doesn't work for me (It gets removed from the database but not from the hard drive)

I also noticed that if you click the permanent delete icon with no scene's checked from the tag/delete multiple scene's checkboxes, it still generates the warning popup but it doesn't remove anything

sjclayton commented 8 years ago

@thierry400 Deleting from disk does work with at least one item checked (I confirmed that it worked before pushing the updates) -- It's still a requirement for now to have at least one item checked...

Also yes, the popup warning is still generated even with no items checked -- this needs to be fixed at some point... I don't know how to prevent it from happening with the way the code is..

sjclayton commented 8 years ago

@thierry400 can you post your console output when deleting a file from disk with one item checked..?

Just make a copy of a random file from your collection and then delete the one that is in the database, and then you can use the copy to replace it afterwards...

thierry400 commented 8 years ago

Indeed sorry, I forgot to post the console code.

Here is the code I get when i try to perm delete a file:

Patching scene permDelete true Got OSError while trying to delete Z:\Overige\Videos\008_Bianca_I.wmv : <member 'errno' of 'OSError' objects> C:\Yapo\YAPO\videos Deleted 'videos\media\scenes\7' [08/Sep/2016 01:46:31] "POST /tag-multiple-items/ HTTP/1.1" 200 0

I checked with my file browser and the video is still existing

thierry400 commented 8 years ago

I deleted a few other scene's before and I always got the OSError and the files never got deleted from the computer

sjclayton commented 8 years ago

@curtwagner1984 will have to look into this... it is working fine on my end (like I said, I checked it before I pushed the updates)

sjclayton commented 8 years ago

@thierry400 so it was happening then (wasn't deleting the files from the disk for you) before I even added the confirm popup then...?

EDIT: Sounds like it is a file permissions problem on your end potentially... because YAPO is actually trying to delete the file from disk, based on the console output you posted...

thierry400 commented 8 years ago

I'm not sure if it deleted the files before the confirm popup got added. I then just assumed it did.. I diden"t check it back then

sjclayton commented 8 years ago

@thierry400 I was just going based on what you said...

thierry400 said:

I deleted a few other scene's before and I always got the OSError and the files never got deleted from the computer

thierry400 commented 8 years ago

I meant earlyer today.

And The permissions problem could very likely be it yes. I guess it doesn't realy help that the files aren't located on my C:/ drive

sjclayton commented 8 years ago

@curtwagner1984 @thierry400 I just ran some more testing on it.... and deleting a scene from disk works for one single item without it being checked (didn't think that it was supposed to work that way, even with the modifications) and multiple item file deletion when multiple items are checked works as well on my end...

So like I said before.... it may very well be a permissions issue on your end...

thierry400 commented 8 years ago

hmm then I guess it will be some problems on my end. Next week I have some more time. I might try a few things to see if I can get it working somehow then

curtwagner1984 commented 8 years ago

@thierry400 I added a more verbose error log, it should now print the exact error so we could better understand what is happening.

You had permission issues before, no? Maybe just run the server with admin privileges ?

thierry400 commented 8 years ago

I updated Yapo and tried it again. It now gives this in the command line:

Patching scene permDelete true Got OSError while trying to delete Z:\Overige\Videos\Desertigl - Public Nylon Foot job.mp4 : Error number:<member 'errno' of 'OSError' objects> Error Filename:<member 'filename' of 'OSError' objects> Error:<member 'strerror' of 'OSError' objects> C:\Yapo\YAPO\videos Deleted 'videos\media\scenes\1283' [08/Sep/2016 02:40:22] "POST /tag-multiple-items/ HTTP/1.1" 200 0

curtwagner1984 commented 8 years ago

@thierry400 My bad, try agan with latest pull

thierry400 commented 8 years ago

that gives me the following:

Patching scene permDelete true Got OSError while trying to delete Z:\Overige\Videos\tittentrine_-_eigene_pissesaufen-288498.wmv : Error number:13 Error Filename:Z:\Overige\Videos\tittentrine-_eigene_pissesaufen-_288498.wmv Error:Toegang geweigerd C:\Yapo\YAPO\videos Directory 'videos\media\scenes\3604' already deleted [08/Sep/2016 03:05:17] "POST /tag-multiple-items/ HTTP/1.1" 200 0

thierry400 commented 8 years ago

( and Error:Toegang geweigerd means Error:Access denied )

sjclayton commented 8 years ago

@thierry400 @curtwagner1984 .... Permissions issue very likely.... as it's not actually able to execute the operation on the file... (it is trying to delete though)

curtwagner1984 commented 8 years ago

@thierry400 Have you tried running as admin? Also is the file marked as 'read only' ?

thierry400 commented 8 years ago

I noticed my library is kind of a mix from files marked as read only and not. Anyways it doesn't seem to give any diffrent results.

Running as admin gets me a slightly difrent console result but still with the same actual result (removed from db, not removed from disk):

Patching scene permDelete true File already deleted! C:\Yapo\YAPO\videos Deleted 'videos\media\scenes\30' Removed '025_Lunae_Leyre_Sandra_SUB_W' from database [08/Sep/2016 03:44:28] "POST /tag-multiple-items/ HTTP/1.1" 200 0

curtwagner1984 commented 8 years ago

@thierry400 So if you run it as admin, it thinks it successfully deleted the scene, but it didn't? When it says 'file already deleted' it means he gets a 'File does not exist' error. Maybe it's looking for it in the wrong path...

I've added even more verbose logging, it should now say what file it was looking for when it says 'file already deleted'.