Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Give a route for retrying invites for users which may not exist #2434

Merged
merged 3 commits into from
Jan 14, 2019

Conversation

turt2live
Copy link
Member

Fixes element-hq/element-web#7922

This supports the current style of errors (M_NOT_FOUND) as well as the errors presented by MSC1797: matrix-org/matrix-spec-proposals#1797

Note to the reviewer: This is intentionally done against develop instead of experimental so it lands in the upcoming release. It will need merging to experimental if approved. Also sorry for the large diff - I refactored the actual invite logic out of _inviteMore so I could reuse it.

Fixes element-hq/element-web#7922

This supports the current style of errors (M_NOT_FOUND) as well as the errors presented by MSC1797: matrix-org/matrix-spec-proposals#1797
@turt2live
Copy link
Member Author

CI is failing due to lint errors fixed on experimental. To ease the merge conflict, I'd recommend pretending they don't exist for now.

@turt2live turt2live requested a review from a team January 11, 2019 05:05
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks along the right lines although the retrying doesn't feel quite right. I'm not sure it makes sense to automatically retry any failed ones with those error codes as it's unlikely to help (could it cause it to retry in a loop?). Also just describing them as 'retries' feels misleading because it's not telling me that it will behave differently when I hit the 'try again' button (ie. if I saw that, I would assume mashing 'try again' was pointless as it was not going to make the user appear out of nowhere).

I think I would probably fix this by making the dialog specific to only profile lookup failures, ie. only display it for errors where our profile lookup phase failed, make the copy refer to it as inviting these users anyway and make the option, "alwaysInviteUsersWithNoProfile" or something (which also should still work for a future 'show question marks' version of this).

src/utils/MultiInviter.js Outdated Show resolved Hide resolved
src/utils/MultiInviter.js Outdated Show resolved Hide resolved
@turt2live
Copy link
Member Author

Loops shouldn't be possible as the inviting logic has been broken out in a way where it won't get caught by the stuff that says the lookups failed.

I'll update the dialog, etc to care more about showing profile errors (non-fatal). The "retry" wording is indeed a lie as it doesn't truly retry - it skips a bunch of checks and goes ahead with the invite.

@turt2live turt2live requested a review from dbkr January 11, 2019 22:53
@turt2live turt2live assigned dbkr and unassigned turt2live Jan 11, 2019
@turt2live
Copy link
Member Author

(tests are still failing due to other lint errors fixed on experimental)

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - this looks like it would confuse me a lot less :)

@turt2live turt2live merged commit a324035 into develop Jan 14, 2019
@turt2live turt2live deleted the travis/invite-better branch January 14, 2019 14:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants