[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> |
| messages.error(request, 'No file found with the provided guid.') | ||
| return redirect(self.get_success_url()) | ||
|
|
||
| file = guid.referent |
There was a problem hiding this comment.
Don't we need to verify that the guid referent is actually a File and not another project, user, etc.?
There was a problem hiding this comment.
yep, you are right
| messages.error(request, 'No file found with the provided guid.') | ||
| return redirect(self.get_success_url()) | ||
|
|
||
| file = guid.referent |
| 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)
2 participants
https://openscience.atlassian.net/browse/ENG-10283