Add 'Remove' button to shared projects

[Bubballoo3]

Fixes #4841. Replaces the 'Delete' button of shared projects with 'Remove' to more accurately indicate what is happening (since we have decided that you will not delete others' projects with a button). Since Project#destroy! still does half of what we want (removing from the lookup table), we still use the delete method instead of adding a new route.

We use the same check deletable? to differentiate these cases whether serving buttons, confirmation messages, or whether destroy! actually deletes the project files. The deletable? method currently only checks whether CurrentUser owns the project, but this logic can easily be changed if we decide on a different criteria later on.

[johrstrom]

Seems fine to me.

[Bubballoo3]

It is also worth mentioning that this currently changes the appearance of projects that are deleted through the filesystem, since deletable? defaults to false if the configuration_directory cannot be found. This should be remedied in #4840, when nonexistent projects are not shown in the index at all, but this is perhaps worth checking back on after these are both merged (or waiting to merge this until after #4840)

[Bubballoo3]

This resolves an error I realized in testing, where as soon as Project#destroy! is called on a deletable project, it turns deletable? == false and then serves the wrong notice. This is fixed by making sure all calls to deletable? happen before the destroy action.

[johrstrom]

Seems like you need to leave a comment for the same in case we forget this detail.

[Bubballoo3]

This extra line accounts for the fact that remove_from_lookup is a safe operation, while FileUtils#remove_dir is unsafe. So when remove_from_lookup fails, we have to manually raise a blank StandardError, which has "StandardError" as its message. The error is already added to the object when remove_from_lookup is rescued, so we don't need to pass the error details up.

In contrast, FileUtils#remove_dir does not have a chance to add its errors to the object, so we do that here.

[johrstrom]

That sounds like a code smell to me. Seems like we need to refactor this so it just works in a simple and correct way without checking for specific conditions like this.

[Bubballoo3]

The smell is definitely from the mixing of safe and unsafe methods inside delete!, and we can basically go two ways with it.

The first approach would be to add a private Project#remove_dir method that executes FileUtils#remove_dir in the same safe pattern as remove_from_lookup, returning false if unsuccessful and adding its errors to the object. Then project#destroy is a safe boolean function, and we collect any errors from the object uniformly in the controller.

The second approach is to keep destroy! an unsafe method, and modify remove_from_lookup to be unsafe (and rename to remove_from_lookup!. That way we call destroy! the same way in the controller, but don't have to deal with the special case of errors being added to the object somewhere else.

There is maybe a third case where we raise any errors on the object during the rescue clause of destroy!, (and don't read the object's errors in the controller) but this would only get one of the errors on the object raised, and generally feels like dodging the problem instead of picking a side.

Any preference one way or another?

[Bubballoo3]

I think the second one is probably ideal, given that remove_from_lookup is not used anywhere else

[Bubballoo3]

I ended up implementing the second approach above. This greatly simplifies the error handling we have to do, but does slightly decrease the verbosity of the messages. Now every error encountered during destroy! announces with the generic error message ('Error processing your request'), instead of specifying 'Could not update lookup'. If we want to keep that verbosity, we can go with the first option, but handle/raise the errors in destroy! so that we keep the messages. Seeing as the only errors we can encounter are file access, this lack of verbosity isn't so bad, because we at least get the path in all of those messages.

[johrstrom]

There is maybe a third case where we raise any errors on the object during the rescue clause of destroy!,

If you really want the verbosity I think rescuing and re-raising a specific error class is the way. Then in turn, rescuing that specific error class and updating errors as appropriate.

Though this makes it verbose in both directions - in the code base and also to the end user.

UpdateLogError < StandardError
RmDirError < StandardError

rescue e => UpdateLogError 
  # errors.add() for updating the log
rescue e => RmDirError 
  # errors.add() for removing the directory
rescue e => StandardError
  # errors.add() generically 

[Bubballoo3]

Yeah it seems to me that the simplicity gained from doing it the way I have is worth losing that verbosity, do you agree?

[Bubballoo3]

The other reason I don't think this matters too much is that despite being unsafe methods, both of these are very straightforward and there is not much room for things to go wrong. remove_from_lookup only reads and writes your personal lookup file, which should be pretty dependable, and the rm_dir call only happens if we have verified that you own the directory you are about to delete. So three file actions total, all on files that you should own, and you can tell what piece is failing because (even) the generic error points you to the troublesome file.

[johrstrom]

Yeah it seems to me that the simplicity gained from doing it the way I have is worth losing that verbosity, do you agree?

I haven't looked at it yet, just responded. I thought I'd just mention it. As a knee-jerk reactions- I'd put the needs of the end users (getting good messages) above ours (simplicity in code) in general, but I'd have to look closer at what the end user sees here specifically.

[Bubballoo3]

As a knee-jerk reactions- I'd put the needs of the end users (getting good messages) above ours (simplicity in code) in general

That is definitely my first reaction as well, which is why I want to have a good explanation for not doing it. The gist of my argument is that the user likely won't care whether an error happens during remove_from_lookup or rm_dir, they just need to see the file and the missing permission to be able to solve the issue.