Provide a more informative error message when navigating to Not Available able resources

[lalit97]

when navigating to a Not Available partner. Instead of raising a generic "Not found" error, we should explain that this partner isn't currently open for applications.

Bug: T212767

[Samwalton9]

Could you add a screenshot of what this looks like? This is simpler so shouldn't be a problem to merge it :)

[Samwalton9]

Actually I've just noticed, it looks like you added the text to the wrong view. We want to modify PartnersDetailView to raise an error message. The behaviour of the partner not being available is defined in get_queryset in that view. I'm actually not personally sure how to proceed with adding an error message when that function returns an unavailable partner.

[lalit97]

Actually I've just noticed, it looks like you added the text to the wrong view. We want to modify PartnersDetailView to raise an error message.

Yes you were right. I didn't understand the issue previously and on finding an empty Http404 object, I had added the message in it without giving a thought.

I'm actually not personally sure how to proceed with adding an error message when that function returns an unavailable partner.

I think the only way was to override the get() function in our PartnersDetailView. Because it automatically raises a Http404 if the object is not found. So if we like to put a custom error message we have to override it.

[lalit97]

Taking a test case, I have put the status of the Partner with id=18 as Not available in the test database.

The PartnersFilterView behaves differently depending upon the logged in user.

(1) If the user is staff member it will list the not available partners at localhost/partners.
(https://github.com/lalit97/TWLight/blob/master/TWLight/resources/views.py#L34)
Now If a staff member tries to access the detail page of not available Partner, then it will show the custom message as described here
https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/resources/views.py#L59

(2) In case a normal user or coordinator goes to localhost/partners the Partner with id=18 will not be listed there.
(https://github.com/lalit97/TWLight/blob/master/TWLight/resources/views.py#L45)

Though if anyone access it directly through url, and goes to localhost/partners/18

before my changes it shows the normal 404 page.

And after adding the changes I have made it will show the custom error message.

[Samwalton9]

Great job figuring this out!

One question - does this show the same message if a user attempts to navigate to, say, /partners/1000000? (i.e. a partner that doesn't even exist in the database)

[lalit97]

Hello @Samwalton9 currently it is showing the same message for /partners/1000000.

i.e. a partner that doesn't even exist in the database

What should be the ideal behaviour in this case?
should we display a normal 404 not found message?

[Samwalton9]

Ideally, yes. That's a 'true' 404 because there actually isn't anything there. Is there an obvious way to handle this case?

[lalit97]

Is there an obvious way to handle this case?

Yes, I think adding an extra check in except block for the case when partner object is NOT_AVAILABLE will do the trick for us. Made changes according to the idea and it is dealing with all the cases properly :)

One question - does this show the same message if a user attempts to navigate to, say, /partners/1000000

pointing out such cases really makes things much more interesting, helped me to get a more clear view about the issue. Thanks :D

[lalit97]

Attaching some screenshots showing the current changes

(1) /partners/18 (which is marked NOT_AVAILABLE in my test database) showing a custom 404 message.

---

(2) /partners/1000000 showing the default 404 message made for Partner model.

[Samwalton9]

Very nicely implemented!

This looks good to go to me, but I'll leave it to @jsnshrmn as to when this should be merged. Per my comment on another PR, we have quite a few big moving pieces right now so I'm not sure if merging this to master right now is a good idea.

[lalit97]

Very nicely implemented!

Thank you :)

we have quite a few big moving pieces right now so I'm not sure if merging this to master right now is a good idea.

Okay, no problem :)

[jsnshrmn]

This should raise a 403 instead of a 404.

[uyscuti-wiki]

Hi, thanks for working on this, looks good! One tiny little thing for you to take a look inline.

[lalit97]

This should raise a 403 instead of a 404.

made changes according to your suggestions, thank you :)

[suecarmol]

Hello @lalit97! A test is failing due to the changes you made. I think you just need to change test_visibility_of_not_available_1 to assert that a 403 error is raised instead of a 404.

[lalit97]

Hello @lalit97! A test is failing due to the changes you made. I think you just need to change test_visibility_of_not_available_1 to assert that a 403 error is raised instead of a 404.

Thank you for the review @suecarmol. I have fixed the test. Your suggestion made it very easy for me :)

[jsnshrmn]

thanks for your contributions @lalit97!

[lalit97]

thanks for your contributions @lalit97!

my pleasure as always :) Thank you :)