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

Change log server URL locally #2852

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

Conversation

avazirna
Copy link
Contributor

@avazirna avazirna commented Sep 17, 2024

Summary

This PR adds a new option to the Developer Options menu that allows users the set a new URL for where CommCare logs should be submitted.

Ticket: https://dimagi.atlassian.net/browse/SAAS-15980

Product Description

Users are able to set the URL by typing it in the text box and remove it by leaving the text box blank or with just the default value, which is the protocol https://.

Safety Assurance

  • I have confidence that this PR will not introduce a regression for the reasons below
  • Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Safety story

Local tests were successful but this should also be tested by QA.

@avazirna avazirna changed the title Change log server URL to be set locally Change log server URL locally Sep 17, 2024
@avazirna avazirna added this to the 2.54.1 milestone Sep 17, 2024
Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

Few points/questions -

  • Can we add a link to the ticket or github issue which is prompting the change
  • Are we clear and ok with set of steps one need to take to see this setting in dev options ? (Dev options visibility defer in debug and release builds)
  • Have we confirmed with the client that this solution works with them ?
  • Is it urgent enough for the client to include in a hotfix ?

@@ -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

|| 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

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.

@avazirna
Copy link
Contributor Author

Few points/questions -

  • Can we add a link to the ticket or github issue which is prompting the change

Done

  • Are we clear and ok with set of steps one need to take to see this setting in dev options ? (Dev options visibility defer in debug and release builds)

Good point, I may have to whitelist this option, let me check.

  • Have we confirmed with the client that this solution works with them ?

I got sign-off from this comment about this option. And I think we are still waiting for the client's response about it here.

  • Is it urgent enough for the client to include in a hotfix ?

Given the context around this client, I believe this is the bare minimum to satisfy their request and considering that they are in the middle of investigating the issues, it does seem relevant enough to take this opportunity to roll it out now.

@ctsims
Copy link
Member

ctsims commented Sep 19, 2024

@avazirna I think it's worth confirming briefly that this option can cleanly be set to a different URL, used, then have the custom text deleted, and make sure that the submission reverts to the original receiver. With settings like this where the option is a String value or an empty value, there's always a risk that a blank string setting is treated differently than a null or empty value.

@avazirna
Copy link
Contributor Author

@avazirna I think it's worth confirming briefly that this option can cleanly be set to a different URL, used, then have the custom text deleted, and make sure that the submission reverts to the original receiver. With settings like this where the option is a String value or an empty value, there's always a risk that a blank string setting is treated differently than a null or empty value.

Yes @ctsims, I tested that scenario a couple of times and also added a listener to remove the preference when blank

@avazirna
Copy link
Contributor Author

@damagatchi retest this please

@avazirna
Copy link
Contributor Author

@damagatchi retest this please

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

Successfully merging this pull request may close these issues.

3 participants