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

Replace all host and Repository.displayHost with Repository.host #445

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Marc-Andrieu
Copy link
Contributor

@Marc-Andrieu Marc-Andrieu commented Oct 20, 2024

Fix #444
However, the decode argument in getOne seems useless: now it's an unused argument, and its only appearance was here

if (host == displayHost || decode) {

which evaluates to true || decode which is true no matter the value of decode

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 53.88%. Comparing base (318f0b3) to head (94285f4).

Files with missing lines Patch % Lines
lib/tools/repository/repository.dart 11.11% 8 Missing ⚠️
lib/tools/repository/logo_repository.dart 0.00% 7 Missing ⚠️
lib/admin/repositories/group_repository.dart 0.00% 1 Missing ⚠️
lib/auth/providers/openid_provider.dart 0.00% 1 Missing ⚠️
lib/auth/repository/openid_repository.dart 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #445      +/-   ##
==========================================
+ Coverage   53.82%   53.88%   +0.05%     
==========================================
  Files         168      168              
  Lines        3727     3723       -4     
==========================================
  Hits         2006     2006              
+ Misses       1721     1717       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Marc-Andrieu
Copy link
Contributor Author

Making host a static string makes it impossible to override, so Centralisation is broken.

I hesitate between three solutions regarding the short file https://github.com/aeecleclair/Titan/blob/main/lib/centralisation/repositories/section_repository.dart:

  • Not extending it from Repository in the first place: the Repository class is too complex (tokens, host, endpoints, etc), because it's made to handle the responses of a complete API. Here we just want to get a JSON file.
  • Still extending it from Repository, but using native http.get instead of getOne. But some exception handling should be written for Centralisation. Also, not using getOne in this short file makes it useless to extend from Repository
  • Asking Hyperion to get it for us. The only negative part is that it may be a bit slower for the UX. But we would keep using getOne, and https://centralisation.eclair.ec-lyon.fr would not be hard-coded anymore (well actually it's also in the constant imagePath)

@Marc-Andrieu Marc-Andrieu added centralisation This PR works on the centralisation module help wanted Extra attention is needed core This PR change the core labels Oct 20, 2024
@Marc-Andrieu
Copy link
Contributor Author

Marc-Andrieu commented Oct 20, 2024

BTW, notice the rare cases of requests not to Hyperion (namely for the fonts, Centralisation's SVG files, for Plausible, etc) do not use methods of Repository so the 1st solution seems OK (and there are few possible errors when not receiving the JSON).

@Marc-Andrieu Marc-Andrieu added ready for review This PR is ready to be reviewed fix bug This PR fix a bug and removed help wanted Extra attention is needed labels Nov 8, 2024
Copy link
Member

@maximeroucher maximeroucher left a comment

Choose a reason for hiding this comment

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

The last option is best in terms of practices, however, it will require a lot f work for only one endpoint, which is a bit overkill.
Consider going for it if multiple services a being handled this way (we already change the cinéma module for instance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
centralisation This PR works on the centralisation module core This PR change the core fix bug This PR fix a bug ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove displayHost-related stuff
3 participants