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

Remove setting Authenticate before each read #3183

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

Navid200
Copy link
Collaborator

This PR permanently enables the setting "Authenticate before each read" on the G5/G6/Dex1 Debug Settings page, and removes the setting.
There are individuals who disable it and wonder why they have trouble.

This image shows what the settings page looks after:
Screenshot_20231113-142044

Selecting different Libre options, the logs show the exception the same as selecting G6.
Screenshot_20231113-141719

@jamorham
Copy link
Collaborator

So there are a number of problems with this PR. I will go through them below:

  1. This doesn't affect the correct key which is always_get_new_keys yours is targeting the string value which is incorrect.
  2. I don't believe this will have any influence on the OB1 collector and I think will apply only to the defunct non-ob1 collector for G5 so I don't think there could be operational problems for users regardless of this setting. But tell me if you think that is incorrect?

Having said that I think it is okay to remove this setting as it is needless clutter for something users shouldn't change and applies to an obsolete collector.

@Navid200
Copy link
Collaborator Author

Thanks,

I will fix it.
Thanks for the review.

@Navid200 Navid200 marked this pull request as draft November 28, 2023 15:20
@Navid200 Navid200 marked this pull request as ready for review November 28, 2023 21:36
@Navid200
Copy link
Collaborator Author

I'm not clear about your direction. You say:

Having said that I think it is okay to remove this setting

That's what I wanted to do. To remove the setting.
If this PR removes the setting, please merge it.
If it doesn't, would you please explain a little bit more what I need to change?

@Navid200
Copy link
Collaborator Author

Are you saying I should leave IdempotentMigrations alone and not change it because this setting doesn't really exist?
If yes, I understand and will remove that change to that file if you confirm.

@Navid200
Copy link
Collaborator Author

Navid200 commented Nov 28, 2023

However, please note that I see this setting used in wear.
Please advise.

@jamorham
Copy link
Collaborator

jamorham commented Dec 5, 2023

Hi, please re-read my previous comment. Point 1 explains why the PR wont do you you intend it to do and Point 2 explains that I don't believe there are currently any users affected by the problem you mention as the rationale - but I am not against removing the setting as it is not needed but this PR doesn't do that properly so please re-check your work.

@Navid200
Copy link
Collaborator Author

Navid200 commented Dec 5, 2023

@jamorham I have a screenshot, in the very first post, that shows that this PR removes the setting, which is the intent.

@Navid200
Copy link
Collaborator Author

Navid200 commented Dec 5, 2023

I did recheck my work. I just did again.
Searching for always_get_new_keys, before this PR, I get 11 matches in 6 files.

7 of the 11 are wear.
The remaining 4 are the following:
Screenshot 2023-12-05 090224
Screenshot 2023-12-05 090352
Screenshot 2023-12-05 090432
Screenshot 2023-12-05 090525

In this PR, I am deleting the first, which removes the setting.
And I am forcing it to be true using the migration method.

Please tell me what I am overlooking.

@Navid200
Copy link
Collaborator Author

Navid200 commented Dec 5, 2023

In these PRs, where I remove a setting that is not needed any longer, I follow your approach here as you directed me:
23ccbd1

Please tell me what I have done here that is not the same. Or what I have not done that is needed.

@@ -147,6 +147,7 @@ private static void legacySettingsFix() {
Pref.setBoolean("use_ob1_g5_collector_service", true);
Pref.setBoolean("ob1_g5_fallback_to_xdrip", false);
Pref.setBoolean("always_unbond_G5", false);
Pref.setBoolean("authentificate_before_reading", true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The key here is wrong

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How did I miss that?!
Thanks

@@ -2493,6 +2493,7 @@ private void removeLegacyPreferences() {
// removePreferenceFromCategory("use_ob1_g5_collector_service", "ob1_options");
// removePreferenceFromCategory("ob1_g5_fallback_to_xdrip", "ob1_options");
// removePreferenceFromCategory("always_unbond_G5", "ob1_options");
// removePreferenceFromCategory("authentificate_before_reading", "ob1_options");
Copy link
Collaborator

Choose a reason for hiding this comment

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

the key here is wrong

@Navid200
Copy link
Collaborator Author

Navid200 commented Dec 5, 2023

Corrected the keys. Thanks for catching that.
Screenshot_20231205-114245

@jamorham jamorham merged commit 14568c7 into NightscoutFoundation:master Dec 11, 2023
1 check passed
@Navid200 Navid200 deleted the Navid_2023_11_13 branch December 13, 2023 15:39
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.

2 participants