bookwyrm-social / bookwyrm

Social reading and reviewing, decentralized with ActivityPub
http://joinbookwyrm.com/
Other
2.26k stars 266 forks source link

Check if links already exist when adding them to books #1850

Closed mouse-reeve closed 2 years ago

mouse-reeve commented 2 years ago

While a domain is still pending, people may add links multiple times because they aren't shown on the page. It would be helpful to let the user know that the link is already added and pending approval (or blocked)

willhoh commented 2 years ago

I would like to try to work on this problem. Please do not take me into consideration if you have an idea and the will to work on it.

@mouse-reeve, I have a few questions about the issue. Should the form show the current status of the link (pending / blocked)? Or should it only show that the link cannot be added at the moment? As far as I could determine, users can enter the same link several times in the list (at least I could do that in the development environment). I assume this is also not wanted and should be prevented?

willhoh commented 2 years ago

I added the clean() function and I also get an expected message. However, unfortunately the form is somehow statically overlaid on the web page and the cancle button also disappears. Can anyone give me a tip, I'm totally stuck here right now.

class FileLinkForm(CustomForm):

    def clean(self):
        """make sure the url isn't pending or blocked"""
        cleaned_data = super().clean()
        url = cleaned_data.get("url")
        domain = urlparse(url).netloc
        if domain and models.LinkDomain.objects.filter(domain=domain).exists():

            status = models.LinkDomain.objects.get(domain=domain).status
            if status == 'pending':
                self.add_error("url", _("Domain already pending. Please try later."))

            elif status == 'blocked':
                self.add_error("url", _("Domain is blocked. Don't try this url again."))
willhoh commented 2 years ago

Hm. Well, as far as I understand it now, it's because of the "modal" and how it is generated. Unfortunately, I don't understand "modal" at all yet. What I do know or have found out is that every failed validation leads to the same result. So it doesn't matter if with my code or without. As soon as you make a wrong input you get a wrong or broken form and the "cancle" button is missing. However, because this gives me no peace, I would be overjoyed if someone would steer me in the right direction.

mouse-reeve commented 2 years ago

Sorry for leaving you hanging -- I'm in a hectic time right now so I'm not quite as responsive as I'd like to be, so thanks for being patient with me. But let's get you some peace!

I like your approach of checking the status of the domain in the form validation. One tweak I'd make is to check if the domain is blocked even if the link is not a duplicate.

I'm not sure I understand the source of your problems with the html rendering. Would you be down to open a draft pull request with your work so far?

Bigger picture, the template system (which uses the default django template engine) compiles a variety of re-usable components and snippets and renders them with data from the view to produce a single HTML file. Modals are a component, which just means that it's an html layout that is used over and over, so that we don't have to re-write the html every time.

Since you're adding form validation, which the add link flow already uses, you should be able to show the errors that the form produces along with existing errors, without adding too much (if any) html. Try, for example, adding a link that passes validation in the browser but not in the python form validation, like https:/www.example.com -- you'll get to a view of the modal with an error message for the user.

Does that help?

willhoh commented 2 years ago

Hi,

One tweak I'd make is to check if the domain is blocked even if the link is not a duplicate. As I could test that, I think I already check exactly that in my code. Because I only check the status of the domain and not the link.

In the following pictures you can see the history of a wrong input / pending domain input. You can see that in the first image the book is visible in the background and also the cancel button is present. On the 2nd image the error message appears, but the book is no longer in the background and the cancel button is gone. As I have already written, you can do this without my code if you enter the URL incorrectly (see third image).

I tested it as described in "Developer Environment". As domain I use http://localhost:1333

image

image

image

I just noticed that you use a translation for the error message. I will try to include it in my code.

mouse-reeve commented 2 years ago

I see! What's happening there is that rather than opening the modal in javascript on the same page, a new page is opened with the modal on it. This is expected behavior -- it allows the site to work without javascript enabled. There's nothing in the background because it's a dummy backdrop, and loading the book data would be unnecessary, since leaving the modal leaves that page. But you're right to observe that the cancel button shouldn't disappear! That said, I don't think that should be part of this issue (and I see that you opened another issue for that, thank you).

For this, let's just check for existing links, and the modal display can be a separate concern.