[MAGNOLIA-4043] page editor, rendering: Previewing a page excluded on a given channel should display a nicer message instead of 404 servlet container page Created: 02/Mar/12  Updated: 28/Apr/14  Resolved: 25/Apr/14

Status: Closed
Project: Magnolia
Component/s: bundle, page editor
Affects Version/s: None
Fix Version/s: 4.5.19

Type: Improvement Priority: Major
Reporter: Federico Grilli Assignee: Christoph Meier
Resolution: Fixed Votes: 0
Labels: support, ux
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: HTML File 404.html     PNG File 404.png    
Issue Links:
Cloners
is cloned by MAGNOLIA-5754 CLONE - page editor, rendering: Previ... Closed
causality
caused by MAGNOLIA-4299 Rendering pages excluded for the curr... Closed
relation
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Description   

Steps to reproduce the issue on stk demo-project (author):

  • go to about page
  • open properties->Output Channels tab and exclude the desktop channel from serving the current page
  • switch to desktop preview

The servlet container default 404 error page is shown which is fine if you're on a public instance but on the author looks more like a bug and force editors to use the browser's back button to go back to Magnolia.
The same thing happens of course for smartphone and tablet channels but in that case it looks less like a Magnolia bug as the 404 page is displayed inside the mobile preview iframe and one can use esc key to go back to the authoring mode.



 Comments   
Comment by Federico Grilli [ 02/Mar/12 ]

Adding a nice 404 error page to web.xml in magnolia-empty-webapp could be a solution, something like

<error-page>
 <error-code>404</error-code>
 <location>/docroot/404.html</location>
</error-page>
Comment by Federico Grilli [ 02/Mar/12 ]

attached an 404 html page (and a screenshot of it) I could come up with my limited design skills.

Comment by Andreas Weder [ 05/Mar/12 ]

You've asked me for a quick review - I think this is fine, the way it is and looks (except maybe for the missing comma between "browser" and "but" ).

I'd prefer a solution, which would distinguish between a 404 and a page not being available due to it not being published on a particular channel. This would allow us to show a precise error message and possibly additional information along with it (e.g. "the page is only available on channels A and C"). But this works for now.

Comment by Christoph Meier [ 23/Apr/14 ]

In agreement with Andreas i added a simple error-handler for 404 to the web.xml and a static page to docroot (as proposed by Federico).
To differentiate the "this-page-does-not-exist-on-this-channel"-case from other 404-cases, we should generally think about what errors we want to handle by default and what error-code we should use for "this-page-does-not-exist-on-this-channel".

Comment by Zdenek Skodik [ 23/Apr/14 ]

404 pages are pretty common subject of change, could we perhaps also make the rendering filter to log something along by sending the error code? This should help with narrowing down issues caused by misconfigured channels.

Comment by Christoph Meier [ 24/Apr/14 ]

I changed RenderingFilter#doFilter according to Zdenek's input; e.g. it will log now:
The requested resource ('/demo-project/about.html') is not available on channel 'desktop'

Comment by Roman Kovařík [ 25/Apr/14 ]
log.info("The requested resource ('"+aggregationState.getCurrentURI()+"') is not available on channel '{}'", aggregationState.getChannel().getName());
  1. This line doesn't seem to be covered RenderingFilterTest. Could you please add unit test to cover it?
  2. Missing spaces (Do you use our formatting style? ).
Comment by Christoph Meier [ 25/Apr/14 ]

Added a unit-test (to ensure it's logged if a resource is not available in the requested channel); ensured proper formatting.
(See https://git.magnolia-cms.com/gitweb/?p=magnolia_main.git;a=commit;h=b3f07add9fa8afd599e0e5e4b82589fd8229c13c .)

Comment by Roman Kovařík [ 25/Apr/14 ]

Sorry for reopening again :
We should prefer usage of matchers instead of single assertions according to this knowledge transfer https://wiki.magnolia-cms.com/display/DEVINT/Knowledge+Transfer#KnowledgeTransfer-Testingtechniques-TestingwithMatchers-session1
Something like:

- assertTrue(stringWriter.toString().indexOf(logForChannelExclude404) > -1);
+ assertThat(stringWriter.toString(), containsString("is not available on channel"))

The reason is that you get more descriptive error message when the test fails. Also we don't need to match whole sentence so we don't need to change tests when a small unimportant part of a message is changed.

Comment by Christoph Meier [ 25/Apr/14 ]

This ticket should be ported to 5.2x and 5.3, too.
Since it is not possible just to cherry-pick, i create a clone / follow-up which then can be resolved for 5.2.5 (on next iteration) and 5.3, which allows to set this one to integrated status.
Clone: => http://jira.magnolia-cms.com/browse/MAGNOLIA-5754

Generated at Mon Feb 12 03:52:07 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.