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.

Sign up for GitHub

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

Jump to bottom

Change log server URL locally #2852

Open
wants to merge 3 commits into
base: commcare_2.54
Choose a base branch
from
Open

Change log server URL locally #2852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c16080
Add option to set logs server URL in Developer Settings
avazirna Sep 17, 2024
91acfd7
Remove key when logs URL is blank
avazirna Sep 17, 2024
dd17c2c
Whitelist logs URL developer option
avazirna Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,4 +456,5 @@
<string name="fcm_default_notification_channel">notification-channel-push-notifications</string>
<string name="app_with_id_not_found">Required CommCare App is not installed on device</string>
<string name="audio_recording_notification">Audio Recording Notification</string>
<string name="logs_server_url_protocol">https://</string>
</resources>
5 changes: 5 additions & 0 deletions app/res/xml/preferences_developer.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,9 @@
android:entryValues="@array/pref_enabled_vals"
android:key="cc-enable-certificate-transparency"
android:title="Certificate Transparency"/>
<EditTextPreference
android:defaultValue="@string/logs_server_url_protocol"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if user ever sees this defaultValue as an app should always have the log_receiver_url populated ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they see it when log_receiver_url is not set, by default the logs URL is the Form Submission log

android:enabled="true"
android:key="log_receiver_url"
android:title="Log Submission Server"/>
</PreferenceScreen>
15 changes: 15 additions & 0 deletions app/src/org/commcare/preferences/DeveloperPreferences.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.commcare.preferences;

import static org.commcare.preferences.HiddenPreferences.ENABLE_CERTIFICATE_TRANSPARENCY;
import static org.commcare.preferences.ServerUrls.PREFS_LOG_POST_URL_KEY;

import androidx.appcompat.app.AppCompatActivity;
import android.content.Intent;
Expand Down Expand Up @@ -85,6 +86,7 @@ public class DeveloperPreferences extends CommCarePreferenceFragment {
WHITELISTED_DEVELOPER_PREF_KEYS.add(AUTO_PURGE_ENABLED);
WHITELISTED_DEVELOPER_PREF_KEYS.add(ALTERNATE_QUESTION_LAYOUT_ENABLED);
WHITELISTED_DEVELOPER_PREF_KEYS.add(ENABLE_CERTIFICATE_TRANSPARENCY);
WHITELISTED_DEVELOPER_PREF_KEYS.add(PREFS_LOG_POST_URL_KEY);
}

/**
Expand Down Expand Up @@ -120,6 +122,19 @@ protected int getPreferencesResource() {
public void onCreatePreferences(Bundle savedInstanceState, String rootKey) {
super.onCreatePreferences(savedInstanceState, rootKey);
setHasOptionsMenu(true);
EditTextPreference editTextPreference = (EditTextPreference)findPreference(PREFS_LOG_POST_URL_KEY);
if (editTextPreference != null) {
editTextPreference.setOnPreferenceChangeListener((preference, newValue) -> {
if (newValue == null || newValue.toString().trim().isEmpty()
|| newValue.equals(getString(R.string.logs_server_url_protocol))) {
SharedPreferences appPreferences = CommCareApplication.instance().getCurrentApp()
.getAppPreferences();
appPreferences.edit().remove(PREFS_LOG_POST_URL_KEY).apply();
Copy link
Contributor

Choose a reason for hiding this comment

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

think we should be defaulting to the url from app profile xml instead of completely removing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is related to the notion of what is the default URL for the logs submission, right now it seems to be the form submission URL

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah we should be defaulting it to form submission url instead of removing it completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shubham1g5 that would be changing the way this preference is currently used and create redundancy. Right now, it seems that we only set the preference when the logs URL is different from the form submissions URL. Defaulting to the submissions URL and to keep consistency, would require setting it during app installation as well, right?

Copy link
Member

Choose a reason for hiding this comment

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

@shubham1g5 that was my first impulse as well, but I think it's actually too dangerous to set this redundantly for two reasons.

1 - It introduces a new parity requirement. If we changed the submission URL for form submissions on HQ's end, we'd now need to also change the log URL, but we can't really do that with HQ settings because it'll blow away the config for anyone who's added the custom setting.
2 - From a usability perspective, it's dangerous because the users don't have a super meaningful way currently to know what was in the box for this current UI before. If the Log URL is Empty when you load it, it's clear that setting it back to blank should be the "inverse" that sets things back to neutral. If you load up the field then and its got the HQ default form URL already in it, I think you'd naturally then assume that you need to keep track of that URL so you can set it back if you want to undo the action.

Copy link
Contributor

@shubham1g5 shubham1g5 Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Defaulting to the submissions URL and to keep consistency, would require setting it during app installation as well, right?

Makes sense, I didn't realise we don't do it already.

we can't really do that with HQ settings because it'll blow away the config for anyone who's added the custom setting.

I am not sure I understand it. My thought was to do this as a one time action at app install time -

  1. While Installing app, we populate the logs_receiver_url in app preferences by copying it from form submission url.

  2. When user opens the logs url preference to edit, they would see what this url is pointing to (which would be same as form submission url)

  3. User edits the url to some other valid url. We preserve it and show it to user in this preference

  4. User edits the url and change is to a non-valid url (empty or something else) -> We show user an error that the change they made is invalid and preserve whatever the old value of the url was.

No strong thoughts here though overall, though with the current change behaviour it's probably better to rename the field from Log Submission Server to Custom Log Submission Server to make it more clear that existing log server is not empty when they see it empty.

return false;
}
return true;
});
}
}

@Override
Expand Down