[ENG-10283] added ability to add/remove files of an archived registration under osf storage#11674
Conversation
felliott
left a comment
felliott
left a comment
There was a problem hiding this comment.
Looks reasonable. I've added a few nitpicks and some possibly-unhandled error cases. If those error cases are handled elsewhere, then I think you ignore them.
| <a href="{% url 'nodes:search' %}" class="btn btn-primary"> <i class="fa fa-search"></i></a> | ||
| <a href="{% url 'nodes:node-logs' guid=node.guid %}" class="btn btn-primary">View Logs</a> | ||
| {% include "nodes/remove_node.html" with node=node %} | ||
| {% include "nodes/remove_file.html" with node=node %} |
There was a problem hiding this comment.
Nitpick: Can we delete this file? Is it used anywhere else?
There was a problem hiding this comment.
I believe I removed it accidentally because supposed I had to override this file removal workflow
| </div> | ||
| </div> | ||
| <div class="modal-footer"> | ||
| <button class="btn btn-danger" name="action" value="ham" type="submit">Confirm</button> |
There was a problem hiding this comment.
nitpick: value="ham"? Is that needed?
| </div> | ||
| </div> | ||
| <div class="modal-footer"> | ||
| <button class="btn btn-danger" name="action" value="ham" type="submit">Confirm</button> |
admin/nodes/views.py
Outdated
| return redirect(self.get_success_url()) | ||
|
|
||
| file = guid.referent | ||
| registration.files.filter(id=file.id).delete() |
There was a problem hiding this comment.
What if the file exists, but not under the registration?
There was a problem hiding this comment.
If I got it correctly, then nothing will happen as we query files of a specific registration, which means file won't be found. We should delete a file only if it's attached to a registration, not project (because in the project user can manually delete a file unlike in the archived registration, this is the purpose of this PR to add such a functionality for product team)
There was a problem hiding this comment.
d'oh! You're absolutely right, sorry for the noise.
There was a problem hiding this comment.
Resolved too soon. My first message was poorly worded. I meant that if the file isn't under the registration, shouldn't this return an error message? I know it won't delete the file, but I would expect it to complain instead of saying The file was successfully removed.
admin/nodes/views.py
Outdated
| return redirect(self.get_success_url()) | ||
|
|
||
| file = guid.referent | ||
| parent_node = registration.registered_from |
There was a problem hiding this comment.
nitpick: move this line after the file check
felliott
left a comment
felliott
left a comment
There was a problem hiding this comment.
Sorry, one small change needed and one nitpick. I didn't word my previous feedback very well. Apologies!
2 participants
https://openscience.atlassian.net/browse/ENG-10283