Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

Deleting non-existent iPaper document throws error

#2

If one tries to delete an has_ipaper_and_uses model and does not have an ipaper document set on it, an exception is thrown. This is because the ScribdFu::load_ipaper_document method throws an exception rather than returning nil (causing, in turn, the model method ipaper_document to do the same, even though it says it returns null).

There are different ways to fix this issue at different levels. The simplest is to make ScribdFu::load_ipaper_document return nil. Or you could change the condition in destroy_ipaper_document to be if ipaper_id. Depends on the broader implications.

Reported by Matt Darby · April 7th, 2009 @ 06:11 PM

State: resolved
Milestone: none
Assigned to: Matt Darby Matt Darby

Activity

  1. Matt Darby
    Matt Darby

    Is it better to just return nil and log the rescue from Scribd?

    I'm leaning towards keeping the ScribdFuError in this circumstance. I think the end programmer should rescue @ScribdFuError@, as the rscribd gem will throw an error its self if the document isn't found anyway.

    April 7th, 2009 @ 06:35 PM

  2. shai.shefer (at gmail)
    shai.shefer (at gmail)

    Does the same thing happen when using attachment_fu? If so then I would keep it the same way if only for consistency and to make it easier for you. :)

    My initial thought was actually to return nil, because if it's not set why bother but I think keeping the error is the right way to go. If the user explicitly sets a model to use scribd_fu they want everything uploaded to scribd. I know I'd like to know the error existed and try to track down why rather than change it to nil.

    I think in the end, if someone wants all their documents upload to scribd they should be concerned if ipaper fields were no filled in. If they're not, maybe this gem is not the right solution for their app?

    April 7th, 2009 @ 07:13 PM

  3. Matt Darby
    Matt Darby
    • State changed from new to wontfix

    Yep, this issue is base plugin independent. It's only concern is ScribdFu finding the associated iPaper document via Scribd's API. If the document isn't found, then an error should be thrown. Seems simple enough. I'm thinking this should remain as an error.

    April 7th, 2009 @ 07:18 PM

  4. Paul Canavese
    Paul Canavese
    • Assigned user set to Matt Darby

    But what if content type is not scribdable (is not a supported content type)? Then the model is created without an ipaper id. When an attempt is made to delete the model, it will fail.

    April 12th, 2009 @ 05:07 PM

  5. Matt Darby
    Matt Darby
    • State changed from wontfix to resolved

    Good point -- I never thought of that angle. I removed the begin/rescue block from @ScribdFu::load_ipaper_document@, and it will now return nil when the iPaper document doesn't exist.

    2.0.4 will contain this fix.

    April 12th, 2009 @ 07:23 PM

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.