Routing: fix mis-ordered conditional for orgAddress #1457

sohkai · 2020-06-27T17:30:32Z

See https://sentry.io/share/issue/6f218cc4d1434b28b606e23b12b3f743/

Couldn't actually reproduce the error, but on inspection, it was clear the existence conditional (if (!orgAddress)) was placed too low.

Ended up making a slightly larger change, because it felt more clear to nest everything related to the 'org' mode in a code block.

andy-hook

lgtm! perhaps we should add some test cases to cover the various conditions?

Polishes the routing behaviour: - Avoid double slashes (`//`) on home paths - Avoid parsing label data from query strings if a preferences path isn't found

sohkai · 2020-06-29T14:53:52Z

perhaps we should add some test cases to cover the various conditions

Good call! I've added tests for all the exposed internal functions for getting and parsing paths.

In the process, I found a few rough edges and smoothed them out: 36ea8c8.

More importantly, I found the real cause of the bug that is causing all permissions changes at the moment to fail: 7bf1714.

Let me know how this looks—can split out into separate PRs if we want to discuss / work on 36ea8c8 separately!

src/components/SignerPanel/ActionPathsContent.js

sohkai · 2020-06-29T14:56:20Z

src/routing.js

      }

      return getPath({
+        ...locator,


The problem was that we weren't passing along the locator.mode.orgAddress above, causing this to not produce the correct path in the signer panel.

Wasn't sure deep merging was the best approach vs. individually selecting the keys we expect, but felt like the most safe in case we add more keys to locator in the future.

Well found! I think this is fine for now.

I’m wondering if that could be an issue, since it now always extends locator. A change of mode (e.g. from org to onboarding) would copy the entries of the mode A into the mode B, and going the other way will copy entries (which might be undesired) from the mode B back into the mode A.

update() used to have an extend param to let people decide, but we ended up moving this on the consumer side to make it behave always the same: #1272 (comment)

The problem was that we weren't passing along the locator.mode.orgAddress above, causing this to not produce the correct path in the signer panel.

Do you know if the issue is related to mode not being properly extended in an update() call?

In any case, this is probably something that will keep happening, since we have these two nested objects. I think update() should let consumers either set or extend the mode and the preferences, independently or together. We could express this in different ways, but perhaps adding some validation / errors could be enough (e.g. if orgAddress is missing from an update)? Or should we provide a way to set / extend / reset any of the two components?

Edit: wondering if having an automatic inheritance of mode.orgAddress, like we already have for mode.name, would be enough?

Right, as mentioned in #1457 (comment), it sounds like we should prefer to use the function variant of path() when we want to extend an existing mode.

What do you think about staying as close to React.setState()'s behaviour as possible then—if you change an object, you overwrite that object entirely (to the point where we don't even inherit mode.name)?

What do you think about staying as close to React.setState()'s behaviour as possible then—if you change an object, you overwrite that object entirely (to the point where we don't even inherit mode.name)?

That sounds good yes. We could do this and also have errors?

When mode.name is not set.

When mode.orgAddress is not set (org mode).

When mode.instancePath is set without mode.instanceId (org mode).

When mode.status is not set (onboarding mode).

When preferences.subsection is set without preferences.section.

To be clear, we would use log() for the errors?

I was thinking of throwing? To make sure we don’t forget anything when updating the mode.

Closing an offline discussion, we decided to:

Log a warning

On production, send an error to sentry

Move the sentry configuration to be environment variable-controlled

Only initialize sentry on *.aragon.org domains (SENTRY_DOMAIN=*.aragon.org)

Logged warnings in c88176d; will add the sentry bits in another PR.

bpierre · 2020-06-30T22:26:46Z

src/routing.js

+  // Ignore labels if search does not contain a preferences path
+  const labels = searchParams.has('preferences')


bpierre · 2020-06-30T23:58:35Z

src/routing.js

      }

      return getPath({
+        ...locator,


I’m wondering if that could be an issue, since it now always extends locator. A change of mode (e.g. from org to onboarding) would copy the entries of the mode A into the mode B, and going the other way will copy entries (which might be undesired) from the mode B back into the mode A.

update() used to have an extend param to let people decide, but we ended up moving this on the consumer side to make it behave always the same: #1272 (comment)

The problem was that we weren't passing along the locator.mode.orgAddress above, causing this to not produce the correct path in the signer panel.

Do you know if the issue is related to mode not being properly extended in an update() call?

In any case, this is probably something that will keep happening, since we have these two nested objects. I think update() should let consumers either set or extend the mode and the preferences, independently or together. We could express this in different ways, but perhaps adding some validation / errors could be enough (e.g. if orgAddress is missing from an update)? Or should we provide a way to set / extend / reset any of the two components?

Edit: wondering if having an automatic inheritance of mode.orgAddress, like we already have for mode.name, would be enough?

Scope adjusted

…uration

bpierre

💯 💯 💯

Routing: fix mis-ordered conditional for orgAddress

18a7b92

sohkai requested review from bpierre and andy-hook June 27, 2020 17:30

auto-assign bot requested a review from Evalir June 27, 2020 17:33

andy-hook previously approved these changes Jun 29, 2020

View reviewed changes

sohkai added 2 commits June 29, 2020 15:19

SignerPanel: fix routing to permissions on role changes

7bf1714

Routing: round out routing behaviour, add tests

36ea8c8

Polishes the routing behaviour: - Avoid double slashes (`//`) on home paths - Avoid parsing label data from query strings if a preferences path isn't found

vercel bot deployed to Preview June 29, 2020 14:52 View deployment

sohkai commented Jun 29, 2020

View reviewed changes

src/components/SignerPanel/ActionPathsContent.js Show resolved Hide resolved

sohkai commented Jun 29, 2020

View reviewed changes

bpierre reviewed Jul 1, 2020

View reviewed changes

This was referenced Jul 3, 2020

Error when granting permissions #1466

Closed

Unable to change permissions #1469

Closed

sohkai added 2 commits July 6, 2020 20:43

SignerPanel: use routing.path()'s functional update

41b7bc2

Routing: log routing warnings on malformed path or preferences config…

c88176d

…uration

vercel bot temporarily deployed to Preview July 6, 2020 19:07 Inactive

Merge branch 'master' into fix-routing-conditional

12a70d0

vercel bot deployed to Preview July 6, 2020 19:11 View deployment

bpierre approved these changes Jul 6, 2020

View reviewed changes

sohkai merged commit 82f4d5f into master Jul 6, 2020

sohkai deleted the fix-routing-conditional branch July 6, 2020 20:02

This was referenced Jul 6, 2020

Sentry: initialize on app start, and add utility for logging to it #1476

Merged

Add SENTRY_DOMAIN configuration setting #1478

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Routing: fix mis-ordered conditional for orgAddress #1457

Routing: fix mis-ordered conditional for orgAddress #1457

sohkai commented Jun 27, 2020

andy-hook left a comment

sohkai commented Jun 29, 2020

sohkai Jun 29, 2020

andy-hook Jun 30, 2020

bpierre Jun 30, 2020 •

edited

Loading

sohkai Jul 1, 2020

bpierre Jul 1, 2020

sohkai Jul 1, 2020

bpierre Jul 1, 2020

sohkai Jul 1, 2020

sohkai Jul 6, 2020

bpierre Jun 30, 2020

bpierre Jun 30, 2020 •

edited

Loading

bpierre left a comment

		// Ignore labels if search does not contain a preferences path
		const labels = searchParams.has('preferences')

Routing: fix mis-ordered conditional for orgAddress #1457

Routing: fix mis-ordered conditional for orgAddress #1457

Conversation

sohkai commented Jun 27, 2020

andy-hook left a comment

Choose a reason for hiding this comment

sohkai commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpierre Jun 30, 2020 • edited Loading

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

bpierre Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

bpierre left a comment

Choose a reason for hiding this comment

bpierre Jun 30, 2020 •

edited

Loading

bpierre Jun 30, 2020 •

edited

Loading