Feature/detailed review attachments view

[miggol]

Reviewers still needed a more detailed attachments view, for example to display attachment names and comments. This PR should fill that gap.

Additionally I continued working on the comments from #736, which I will further address here.

I have tried to reduce the number of unique classes, however I have failed :-/

Items vs. Slots

As these classes look rather similar and do similar things, it might make sense to just use give slots the right methods and use slots everywhere. The main problem with this is that an item in the documents list does not always represent an attachment. Sometimes it might be the PDF application, METC decision, or what have you. Those files do not live in a slot.

This is why the item class needs to exist as a wrapper around whatever thing is being put in the list.

Slots and items also have different purposes. Slots really are a stepper object. They represent a place to put an attachment, even if it's empty. And the DocItem (and the AttachmentItem wrapper) should represent any kind of document in a list.

Vision

So then when it comes to vision, I guess that I like having small wrapper classes because they make our code less interdependent. This makes it easier to change things down the road, and will hopefully also avoid us having to fix a million things at once in the future.

For example, by having made these kinds of container classes like DocItem when developing the previous documents_list, it was very easy to reuse code for the newer attachments list. The following class does seem a bit frivolous:

class AttachmentItem(DocItem):
    """
    A wrapper for AttachmentsList items that works with Attachment slots.
    """

    @property
    def attachment(
        self,
    ):
        return self.slot.attachment.get_correct_submodel()

    def get_link_url(
        self,
    ):
        return self.slot.attachment.get_download_url(
            self.slot.get_proposal(),
        )

    def get_filename(
        self,
    ):
        return self.attachment.upload.original_filename

But I really like that with just a few glue methods we can reuse existing code. Slots and items serve only slightly different purposes, but I still prefer to have them each be good at their own jobs and then write a bit of glue inbetween.

Other strong similarities / potential merges ion the future

There are strong similarities in how we fetch and sort attachments per attached object in the attachments list and in ProposalAttachmentsList.get_context_data(). That's kind of lame but I'm not yet convinced it's worth mashing these together.

I do think it's kind of attractive to mangle DocList into the stepper. It would be nice to have stepper.slots.per_item and stepper.slots.as_containers functions which we could use in many places. However, because we would still need to somehow shoehorn things like the METC decision into the review sidebar, I have left this as is for now. It's just not worth the effort as things are still developing. But it might be worth doing in the future.

Editing attachments as secretary

This is more of an emergency feature, hopefully it will be far less necessary now that the attachments system is improved. Because it should only be used as an exception, I'm fine with it being the same as the user's edit attachments view.

I also believe that it is good to only have one view to maintain, and it guarantees that there's no mismatch between the slots that the secretary sees and the slots that user sees.

It would be an improvement to remove the stepper from this page. But I just don't really see it as worth the effort right now.

Conclusion

• I have moved some methods such as comparable() and is_new() from the Item to the Slot class, but in general I have kept the number of classes the same.
• Wherever I have failed to reduce the number of classes, I have rearranged them in more logical places which will make things more maintainable going forward. For example in their own reviews/utils/attachment_utils.py
• Of course there's also a new detailed attachments view, check it out!

[EdoStorm96]

Nice job. I think it looks good and works nicely. I have some changes I would like to see, but nothing major. See my comments.

Other general remarks:

• I guess the DMP page needs some work. We should probably remove the file field from this page? And ideally move it before the attachments page. But maybe this should be another issue/PR.

• So I would really prefer if the secretary attachments page did not include the stepper and navigation buttons. Can´t we just subclass the attachments view and add a variable to the context, like is_review_stage, and based on this, overwrite the sidebar block (maybe include the review_details_sidebar? Though maybe not.) and override the page_buttons block with a button redirecting to the review_detail? Now there is no good way to get back.

[EdoStorm96]

Unused imports

[miggol]

Done

[EdoStorm96]

Could you run makemigrations? There is a missing migration. Or do you want to do that later?

[miggol]

Done!

[EdoStorm96]

I am kindoff wondering with the legacy documents method. Now, if we have no legacy documents, you add a message: no docs found. But why not just not pass legacy documents to the context? That way, people for whom it's not relevant won't be seeing that whole warning text. I think now, with no legacy documents, it is just confusing.

[miggol]

I think it's still good to show "no documents found" so that there are no trajectories missing from the list if they don't have any documents.

But if there are none at all, the legacy documents list shouldn't show at all either. So I've added an if statement to the review sidebar to make that section hide itself, like similar section in the proposal attachments view already did.

[EdoStorm96]

It looks like there is no way to get back to the regular review_detail, from here? Maybe put a button at the bottom.

[miggol]

I've added a button at the top and bottom of the page. It might be a bit much, but I couldn't really figure out how to style it better.

[miggol]

Alright, thanks for the review. I've addressed all of your comments except for removing the DMP page, for which I've created another issue (#756). It may seem trivial but I would rather get this attachments page on the test server sooner.

At first I thought removing the stepper would be super annoying, because it would involve seven extra URL paths which we would have to remember to maintain every time we change any of the attachment pages. Not to mention the mess in the templates. But after your second request I went looking for an alternative and came up with a mixin solution.

It's super hacky in that it just checks if the current user is secretary and not an applicant/supervisor then overwrites the stepper. But I consider it fit for purpose.