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

feat(SNP-1314): Down by Maintenance #119

Closed
wants to merge 6 commits into from

Conversation

Sadhorsephile
Copy link
Contributor

Checklist

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
    • link issue here (use keywords like fix, close, resolve etc. if necessary)
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR?
  • Have the tests been updated to match the changes in the PR?
  • Attached videos/screenshots demonstrating the fix/feature.
  • Have you run the tests locally to confirm they pass?

New Features

  • Added Down by Maintenance aka Kill Switch as feature

@Sadhorsephile Sadhorsephile added the enhancement New feature or request label May 8, 2024
@Sadhorsephile Sadhorsephile self-assigned this May 8, 2024
@Sadhorsephile Sadhorsephile changed the title feat(SNP-1314: Down by Maintenance feat(SNP-1314): Down by Maintenance May 8, 2024
Copy link

github-actions bot commented May 8, 2024

Link to Pyrus task

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.39%. Comparing base (339c48c) to head (d071643).
Report is 17 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #119   +/-   ##
=======================================
  Coverage   26.39%   26.39%           
=======================================
  Files          55       55           
  Lines         917      917           
=======================================
  Hits          242      242           
  Misses        675      675           

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

Copy link
Contributor

@dzmitry-struk-surf dzmitry-struk-surf left a comment

Choose a reason for hiding this comment

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

Can you rename the feature and the classes into "down_for_maintenance" to be grammatically correct?

Comment on lines +50 to +57
// TODO(init-project): In order to use Down For Maintenance feature you have to:
// - create implementation of [ICheckServerStatusRepository] here
// - call [ICheckServerStatusRepository.configure] and await for it
// - call [ICheckServerStatusRepository.initImmediateCheck]
// - add it to AppScope
// - add DownForMaintenanceWidget in builder of MaterialApp in app.dart
// Otherwise just delete this.

Choose a reason for hiding this comment

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

It is worth putting information about this also in the documentation in the repository itself, and provide link to the documentation here

void initImmediateCheck();

/// Method that configurate service. For example, you can configurate interval of requests.
Future<void> configurate();

Choose a reason for hiding this comment

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

configure() would be a more appropriate name

void initImmediateCheck();

/// Method that configurate service. For example, you can configurate interval of requests.
Future<void> configurate();

Choose a reason for hiding this comment

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

configure() would be a more appropriate name

Comment on lines +11 to +33
Future<void> configurate() async {
/// Here you can configurate your Firebase Remote Config interval of requests for example.
/// For example:
//
// await remoteConfig.setConfigSettings(RemoteConfigSettings(
// fetchTimeout: const Duration(minutes: 1),
// minimumFetchInterval: const Duration(hours: 1),
// ));
//
}

@override
Future<void> initImmediateCheck() async {
/// Here you can make a request to your Firebase Remote Config to check if it is available.
/// For example:
// try {
// await remoteConfig.fetchAndActivate();
// final isServerAvailable = remoteConfig.getBool('is_server_available');
// serverStatus.value = isServerAvailable ? ServerCheckResult.worksNormally : ServerCheckResult.notActive;
// } on Exception catch (_) {
// serverStatus.value = ServerCheckResult.errorOccurred;
// }
}

Choose a reason for hiding this comment

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

It seems that if the method is not implemented, you should add throw NotImplementedException() to the template method body.

}

/// Placeholder widget that shows when server is not available.
/// Replace it with your own widget.

Choose a reason for hiding this comment

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

Sounds like a good place for // TODO(init-project)

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

Successfully merging this pull request may close these issues.

4 participants