Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: package manager failures not showing alerts #3647

Merged
merged 4 commits into from
Sep 23, 2022
Merged

fix: package manager failures not showing alerts #3647

merged 4 commits into from
Sep 23, 2022

Conversation

datlechin
Copy link
Contributor

LoadingModal LoadingModal is not closed when the installation fails

Screenshot 2022-09-17 at 19 11 18

Fixes #0000

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@datlechin datlechin requested a review from a team as a code owner September 17, 2022 12:12
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Change makes sense. Hopefully the request error handling system will show an error alert too? Not seeing that on the screenshot

@datlechin
Copy link
Contributor Author

@askvortsov1 the point is not showing error when validation failed

Screenshot 2022-09-17 at 19 53 35

Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Good catch, for the error though, it would be better to move calling the custom errorHandler within catch of every package manager request so that it can all fallback to the default error handler instead of having to reinvent the wheel. (should have done that when I built this but oh well..)

So basically, it's changing every instance of:

app.request({
  method: ...,
  ...,
  errorHandler,
})

to

app.request({
  method: ...,
  ...,
}).catch(errorHandler)

and adapting the custom error handler if it needs any changes.

@datlechin
Copy link
Contributor Author

datlechin commented Sep 17, 2022

@SychO9 ah, I see, i have try on local
so we no need to use errorHandler on request
it works well

Screenshot 2022-09-17 at 20 51 33

@SychO9
Copy link
Member

SychO9 commented Sep 17, 2022

the custom error handler is necessary for other type of errors which the default one doesn't understand 😇

@datlechin datlechin requested review from askvortsov1 and SychO9 and removed request for askvortsov1 and SychO9 September 19, 2022 15:32
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Thanks 🤗

@SychO9 SychO9 changed the title fix: close LoadingModal when install falied fix: package manager failures not showing alerts Sep 23, 2022
@SychO9 SychO9 merged commit 6e1bc2d into flarum:main Sep 23, 2022
@luceos luceos mentioned this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants