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

Handle server unavailability better #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ashod
Copy link

@Ashod Ashod commented Oct 3, 2020

This prevents xml parsing errors when the server
is not avialable. In a follow-up patch we will
throw a 'server unavailable' error to better
communicate the issue.

@Ashod
Copy link
Author

Ashod commented Oct 3, 2020

@moodlebeuth, @grabs, if you can take a look at this tiny improvement, would be great.

After we merge #20 I will throw an error with the server name stating that it's currently unavailable. But for now, at least we don't show XML errors, which are unhelpful.

This prevents xml parsing errors when the server
is not avialable. In a follow-up patch we will
throw a 'server unavailable' error to better
communicate the issue.
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2020

Codecov Report

Merging #25 into master will decrease coverage by 0.55%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #25      +/-   ##
============================================
- Coverage     14.02%   13.46%   -0.56%     
- Complexity      137      144       +7     
============================================
  Files            17       17              
  Lines           699      728      +29     
============================================
  Hits             98       98              
- Misses          601      630      +29     
Impacted Files Coverage Δ Complexity Δ
moodle/mod/collabora/classes/collabora.php 32.57% <0.00%> (-4.93%) 66.00% <0.00%> (+7.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc22536...c243323. Read the comment docs.

1 similar comment
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #25 into master will decrease coverage by 0.55%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #25      +/-   ##
============================================
- Coverage     14.02%   13.46%   -0.56%     
- Complexity      137      144       +7     
============================================
  Files            17       17              
  Lines           699      728      +29     
============================================
  Hits             98       98              
- Misses          601      630      +29     
Impacted Files Coverage Δ Complexity Δ
moodle/mod/collabora/classes/collabora.php 32.57% <0.00%> (-4.93%) 66.00% <0.00%> (+7.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc22536...c243323. Read the comment docs.

@grabs
Copy link
Contributor

grabs commented Oct 12, 2020

Hi Ashod,

sorry for the long delay.
Thank you for this request!
I looked into it but unfortunately I couldn't get it work as expected :(.
The problem here is that the plugin uses the moodle cache to get the discovery_xml which you try to check.
So if the plugin has established the connection once the xml always is loaded from cache until the cache is purged.
I don't know whether or not the cache is needed. On my dev system the discovery xml is loaded in nearly no time.
For me there are to possible ways to go with this:

  1. You don't use the cache and load the discovery xml each time a user opens a document
  2. You only check the base url and check for an "ok" in it. This should be sufficient too.
    What do you think?

Best regards
Andreas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants