404 support for non existing paths #162

parostatkiem-zz · 2018-10-25T08:37:49Z

Description

Changes proposed in this pull request:

create a helper function to detect unexpected segments in the URL
display an alert (taken from FundamentalUI) when when needed
test the helper function
move existing test files to separate folders (according the code hierarchy)

Related issue(s)

Refeers #58

…04-support-for-non-existing-paths

dariadomagala-sap

Read the comments and add an example to our angular app together with a UI test.

dariadomagala-sap · 2018-10-25T12:25:43Z

core/src/App.html

+	<Backdrop>
+		<div class="fd-page iframeContainer" use:init="context" />
+
+


Maybe I'm picky, but please remove unnecessary spaces ;)

dariadomagala-sap · 2018-10-25T13:16:09Z

core/src/App.html

@@ -226,6 +240,20 @@
              console.error('goBack() not possible, no preserved views found.');
            }
          }
+
+          if ('luigi.displayAlert' === e.data.msg) {
+            if (e.data.detail) {


Would be better to add one additional check to prevent from those undefined errors.
if(e.data && e.data.detail){}

Also, I would vote for changing from 'detail' to 'errorMessage' or something more straight forward. Could be discussed :)

errorMessage looks good to me. message would be even better but it'd collide with the postMessage name which is the same.

dariadomagala-sap · 2018-10-25T13:22:52Z

core/src/App.html

+              this.set({ alertMessage: e.data.detail });
+            } else {
+              console.error(
+                'Error alert has been triggered but no message was provided in "detail" property'


Please change this message. I think it would be sufficient to just let the user know that there was an error, without telling him that the event message has not been sent properly.

So what error should it be?
I thought we may use the alert somewhere else in the future and the developer may forget about the message in an event so this error would tell him directly what the problem is.

I wouldn't show to the user an error saying that developers misconfigured something. That wouldn't tell the user anything, but could cause a confusion. In my opinion it would be sufficient that this information is visible in the console and in the UI there is a more generic message.
We can discuss it with the team.

dariadomagala-sap · 2018-10-25T13:33:53Z

core/test/utilities/helpers.spec.js

+
+    it("should return false when pathSegments numbers don't match", async () => {
+      const tooShortSourceUrl = 'one/two';
+      const tooLongSourceUrl = 'lets/change/Sinon/to/Jest/someday';


Please change this part.

core/test/utilities/helpers.spec.js

dariadomagala-sap · 2018-10-25T13:43:15Z

core/src/utilities/helpers.js

+    return false; //we can already tell segments will not match so no loop is needed
+  }
+
+  mandatorySegments.forEach((segment, index) => {


Maybe it would be good to add an if statement before forEach loop, because if mandatorySegments will be undefined, there will be an error.
It might not be the case for our example, but I'm rather cautious with forEach loops and things like that, they can be tricky 😉

Same with this mandatorySegments.length above etc.

Good point, I'll improve it

…d' checks

…04-support-for-non-existing-paths

dariadomagala-sap · 2018-10-29T13:11:43Z

Code looks better, but please also add an example and UI test as I requested previously.

…04-support-for-non-existing-paths # Conflicts: # core/examples/luigi-sample-angular/src/app/project/project.component.html # core/src/App.html # core/src/services/routing.js

jesusreal

If I enter a url like http://localhost:4200/#/projects-does-not-exist the route is not changed. Tested for both hash and path routing. In this case we should probably redirect to localhost:4200.
The notification still does not show the route the user wanted to navigate to .

…04-support-for-non-existing-paths

…04-support-for-non-existing-paths # Conflicts: # core/src/services/routing.js # core/src/utilities/helpers.js # core/test/services/config.spec.js

jesusreal · 2018-11-15T08:58:23Z

The implementation is not working for a url with query parameters like http://localhost:4200/#/projects/pr2/settings?~foo=bar&

Additionally, there are some refactorings that I would apply. I generated the following patch file you can apply it with this command: git apply patch-404-support-for-non-existing-paths.

All things done by Jesus's patch

…04-support-for-non-existing-paths

jesusreal

Please see my comment

jesusreal · 2018-11-19T09:54:31Z

core/src/utilities/helpers.js

@@ -86,6 +86,36 @@ export const prependOrigin = path => {
  return window.location.origin;
 };

+export const containsAllSegments = (sourceUrl, targetPathSegments) => {


There is still a bug when the user introduces a url with a trailing slash like '/projects/pr2/' (sourceUrl param in function)
Furthermore the implementation could be simpler. My proposal is the following (unit tests are still green after refactoring)

(thanks Jesus)

…04-support-for-non-existing-paths

Handle 404 when route is absolutely not found

parostatkiem-zz added 9 commits October 24, 2018 09:06

Detect the moment when popup should appear

875676f

Implement not-exact-path alert

477e8f2

Handle 404 when route is absolutely not found

e4bd18e

Add min-width to alert

f395863

Rewrite the mechanizm

64414ee

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

b80b7d8

…04-support-for-non-existing-paths

Change CustomEvents to postMessage

b75a50c

Separate tests into directories

d694329

Test the containsAllSegments function

1385ab4

parostatkiem-zz added enhancement New feature or request area/luigi labels Oct 25, 2018

parostatkiem-zz requested review from antiheld, dariadomagala-sap, hardl, jesusreal, kwiatekus, maxmarkus, pekura and y-kkamil as code owners October 25, 2018 08:37

Remove obsolete console.warn()

8389703

dariadomagala-sap self-assigned this Oct 25, 2018

dariadomagala-sap suggested changes Oct 26, 2018

View reviewed changes

parostatkiem-zz added 3 commits October 26, 2018 14:48

Rename error, rename variable in alert event, add some 'anti-undefine…

db71aa4

…d' checks

Correct wrong errorchecking in containsAllSegments, add another test

a6fef26

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

78661a8

…04-support-for-non-existing-paths

parostatkiem-zz added 4 commits October 31, 2018 11:25

Add wrong routes example using linkManager

e3c1499

Add one UI test

af7a078

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

e7c12f6

…04-support-for-non-existing-paths # Conflicts: # core/examples/luigi-sample-angular/src/app/project/project.component.html # core/src/App.html # core/src/services/routing.js

Fix bugs

1b149dd

kwiatekus dismissed their stale review via c1c3d09 November 9, 2018 13:39

kwiatekus approved these changes Nov 9, 2018

View reviewed changes

kwiatekus unassigned dariadomagala-sap Nov 9, 2018

Merge branch 'master' into 404-support-for-non-existing-paths

0b73f82

jesusreal self-assigned this Nov 13, 2018

jesusreal suggested changes Nov 13, 2018

View reviewed changes

parostatkiem-zz added 3 commits November 13, 2018 13:00

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

4abb343

…04-support-for-non-existing-paths

Add links to alerts & redirect to '/' after 404 error

e9798e2

Fix & improve tets

d35f53c

parostatkiem-zz force-pushed the 404-support-for-non-existing-paths branch from 86165ab to d35f53c Compare November 13, 2018 12:54

parostatkiem-zz and others added 6 commits November 14, 2018 09:10

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

f7313fd

…04-support-for-non-existing-paths # Conflicts: # core/src/services/routing.js # core/src/utilities/helpers.js # core/test/services/config.spec.js

aa

ad31069

Change way of handling alert from postMessage to component's state

67adae6

Fix tests

6d17ee0

Remove obsolete line

6f585b5

Merge branch 'master' into 404-support-for-non-existing-paths

e54a288

parostatkiem-zz added 4 commits November 16, 2018 12:27

Remove duplicating tests & remove alert link

306d852

All things done by Jesus's patch

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

a7ccb73

…04-support-for-non-existing-paths

Ignore GET parameters when detecting wrong URL and test it

88d3e2a

Shorten variable name & syntax

8008146

jesusreal suggested changes Nov 19, 2018

View reviewed changes

Simplify containsAllSegments

7092f03

(thanks Jesus)

jesusreal approved these changes Nov 19, 2018

View reviewed changes

parostatkiem-zz and others added 3 commits November 19, 2018 15:02

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

24f7251

…04-support-for-non-existing-paths

Merge branch 'master' of https://github.com/kyma-project/luigi into 4…

2e47937

…04-support-for-non-existing-paths

Merge branch 'master' into 404-support-for-non-existing-paths

d489a59

kwiatekus merged commit 6eb2406 into SAP:master Nov 20, 2018

parostatkiem-zz deleted the 404-support-for-non-existing-paths branch November 28, 2018 08:28

stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021

404 support for non existing paths (SAP#162)

cbbbce4

Handle 404 when route is absolutely not found

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

404 support for non existing paths #162

404 support for non existing paths #162

parostatkiem-zz commented Oct 25, 2018 •

edited

Loading

dariadomagala-sap left a comment

dariadomagala-sap Oct 25, 2018

dariadomagala-sap Oct 25, 2018

dariadomagala-sap Oct 25, 2018

parostatkiem-zz Oct 26, 2018

dariadomagala-sap Oct 25, 2018

parostatkiem-zz Oct 26, 2018

dariadomagala-sap Oct 26, 2018

dariadomagala-sap Oct 25, 2018

dariadomagala-sap Oct 25, 2018

dariadomagala-sap Oct 25, 2018

parostatkiem-zz Oct 26, 2018

dariadomagala-sap commented Oct 29, 2018

jesusreal left a comment

jesusreal commented Nov 15, 2018 •

edited

Loading

jesusreal left a comment

jesusreal Nov 19, 2018

		<Backdrop>
		<div class="fd-page iframeContainer" use:init="context" />

404 support for non existing paths #162

404 support for non existing paths #162

Conversation

parostatkiem-zz commented Oct 25, 2018 • edited Loading

dariadomagala-sap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariadomagala-sap commented Oct 29, 2018

jesusreal left a comment

Choose a reason for hiding this comment

jesusreal commented Nov 15, 2018 • edited Loading

jesusreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parostatkiem-zz commented Oct 25, 2018 •

edited

Loading

jesusreal commented Nov 15, 2018 •

edited

Loading