Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #24855: Allow updating and deleting an existing address. #25031

Merged
merged 4 commits into from
May 10, 2022
Merged

For #24855: Allow updating and deleting an existing address. #25031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cee194f
For #24855: Allow updating info for an existing address.
mcarare Apr 29, 2022
7837fd8
For #24854: Add ability to delete an existing address.
mcarare May 2, 2022
6be7a11
For #24919: Add option menu in the address editor.
mcarare May 3, 2022
938412a
For #24966: Navigate back to previous screen when the last address is…
mcarare May 4, 2022
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
49 changes: 47 additions & 2 deletions app/src/main/java/org/mozilla/fenix/settings/address/AddressEditorFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@
package org.mozilla.fenix.settings.address

import android.os.Bundle
import android.view.Menu
import android.view.MenuInflater
import android.view.MenuItem
import android.view.View
import androidx.lifecycle.lifecycleScope
import androidx.navigation.fragment.findNavController
import androidx.navigation.fragment.navArgs
import mozilla.components.support.ktx.android.view.hideKeyboard
import org.mozilla.fenix.R
import org.mozilla.fenix.SecureFragment
Expand All @@ -27,6 +31,16 @@ class AddressEditorFragment : SecureFragment(R.layout.fragment_address_editor) {
private lateinit var addressEditorView: AddressEditorView
private lateinit var interactor: AddressEditorInteractor

private val args by navArgs<AddressEditorFragmentArgs>()
mcarare marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns true if an existing address is being edited, and false otherwise.
*/
private val isEditing: Boolean
get() = args.address != null
Comment on lines +39 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is out-of-scope for now, as it we're pretty far along in the implementation of this entire feature. Seeing this got me thinking though: why are needing to pass data between simple screens through navigation arguments?

I think we're intending to model the design of this feature after credit cards. However, I believe the intention moving forward is to follow the architecture defined in the lib-state module in A-C.

If we had created a Store that was scoped to this feature (using a StoreProvider to share it between fragments), it would have allowed us to avoid this kind of thing as well as the kind of delegate-chaining we see in the controller/interactor. The resulting side-effects in those classes could have been moved instead into Middlewares attached to the Store.

No actions needed, just sharing some thoughts

Copy link
Member

@gabrielluong gabrielluong May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I believe only AppState is the only Store that persists in memory. So, creating a new Store that is shared between fragments won't actually persist the selected Address between fragments. You would still need to pass the selected Address and inform the newly created Store of the state, which gets recreated every time you navigate to the fragment. My current mental model is that a Store only lives for as long as the fragment does. However, I can totally be wrong about all of this and I have never looked into this deeply enough to confirm these claims - there are people who can provide an answer if we asked in a team channel, which I think would be a nice thing to do.

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 prefer to use nav args when data is passed from a single fragment to a "child" fragment ( by child fragment in this context I define a fragment that is accessible only from the one fragment) and the data is not really needed in other fragments /places in the app. We could save all things in a store/state only for the one screen, but that would be overkill IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you're right, StoreProvider.get is fragment-scoped. My thinking is still being influenced some by my last position, here's some details as to why if you're curious:

The last project I worked on was multi-module, which informed some of the architecture. We typically would have a parent fragment/activity as an entry point to a module, which allowed us to create well-defined entry points as well as share data between child fragments in the module.

For example, imagine having a Settings (or even AutofillSettings) module that had a Store scoped to the lifecycle of a parent fragment.

We could also theoretically add a StoreProvider.get method that used the HomeActivity to share stores between fragments, but I'd worry about memory bloat. We'd probably need to think through lifecycle retention pretty deeply


private lateinit var menu: Menu
Copy link
Member

Choose a reason for hiding this comment

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

I don' think we actually needed this menu variable. Even in CreditCardEditorFragment I didn't think we needed it because it was done so that we can call menu.close() in onPause(), but the menu never really opens, and I think we just ended up copying code from logins without fully understanding 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.

The menu opens if the toolbar text is long enough and the device screen is short. I think it will be good if we keep the same pattern everywhere. Also, we could add some tests for it if we keep it.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. Should we add the menu.close() handling in onPause() in this PR as well? Otherwise, we just have this menu that isn't doing anything currently.


override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)

Expand All @@ -41,18 +55,49 @@ class AddressEditorFragment : SecureFragment(R.layout.fragment_address_editor) {
)

val binding = FragmentAddressEditorBinding.bind(view)
setHasOptionsMenu(true)

addressEditorView = AddressEditorView(binding, interactor)
addressEditorView = AddressEditorView(binding, interactor, args.address)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should pass the address through the bind() instead? This seems cleaner to me instead of having a nullable property in AddressEditorView. Also, it just follows the current CreditCardEditorView convention.

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 would prefer to have the address as property because we also use it in other places (the saveAddress method) and I would prefer not to pass it around as param from method to method.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that makes sense to me.

addressEditorView.bind()
}

override fun onPause() {
super.onPause()
menu.close()
}

override fun onResume() {
super.onResume()
showToolbar(getString(R.string.addresses_add_address))
if (isEditing) {
showToolbar(getString(R.string.addresses_edit_address))
} else {
showToolbar(getString(R.string.addresses_add_address))
}
}

override fun onStop() {
super.onStop()
this.view?.hideKeyboard()
}

override fun onCreateOptionsMenu(menu: Menu, inflater: MenuInflater) {
inflater.inflate(R.menu.address_editor, menu)
this.menu = menu

menu.findItem(R.id.delete_address_button).isVisible = isEditing
}

override fun onOptionsItemSelected(item: MenuItem): Boolean = when (item.itemId) {
R.id.delete_address_button -> {
args.address?.let {
addressEditorView.showConfirmDeleteAddressDialog(requireContext(), it.guid)
}
true
}
R.id.save_address_button -> {
addressEditorView.saveAddress()
true
}
else -> false
}
}
10 changes: 10 additions & 0 deletions app/src/main/java/org/mozilla/fenix/settings/address/AddressManagementFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import androidx.lifecycle.lifecycleScope
import androidx.navigation.fragment.findNavController
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import mozilla.components.lib.state.ext.consumeFrom
import mozilla.components.lib.state.ext.observeAsComposableState
import org.mozilla.fenix.components.StoreProvider
import org.mozilla.fenix.ext.components
Expand Down Expand Up @@ -66,6 +67,15 @@ class AddressManagementFragment : Fragment() {
}
}

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
consumeFrom(store) { state ->
if (!state.isLoading && state.addresses.isEmpty()) {
findNavController().popBackStack()
return@consumeFrom
}
}
}

/**
* Fetches all the addresses from the autofill storage and updates the
* [AutofillFragmentStore] with the list of addresses.
Expand Down
30 changes: 30 additions & 0 deletions app/src/main/java/org/mozilla/fenix/settings/address/controller/AddressEditorController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ interface AddressEditorController {
* @see [AddressEditorInteractor.onSaveAddress]
*/
fun handleSaveAddress(addressFields: UpdatableAddressFields)

/**
* @see [AddressEditorInteractor.onDeleteAddress]
*/
fun handleDeleteAddress(guid: String)

/**
* @see [AddressEditorInteractor.onUpdateAddress]
*/
fun handleUpdateAddress(guid: String, addressFields: UpdatableAddressFields)
}

/**
Expand Down Expand Up @@ -57,4 +67,24 @@ class DefaultAddressEditorController(
}
}
}

override fun handleDeleteAddress(guid: String) {
lifecycleScope.launch {
storage.deleteAddress(guid)

lifecycleScope.launch(Dispatchers.Main) {
mcarare marked this conversation as resolved.
Show resolved Hide resolved
navController.popBackStack()
}
}
}

override fun handleUpdateAddress(guid: String, addressFields: UpdatableAddressFields) {
lifecycleScope.launch {
storage.updateAddress(guid, addressFields)

lifecycleScope.launch(Dispatchers.Main) {
navController.popBackStack()
}
}
}
}
8 changes: 5 additions & 3 deletions ...rc/main/java/org/mozilla/fenix/settings/address/controller/AddressManagementController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,19 @@ class DefaultAddressManagementController(
) : AddressManagementController {

override fun handleAddressClicked(address: Address) {
navigateToAddressEditor()
navigateToAddressEditor(address)
}

override fun handleAddAddressButtonClicked() {
navigateToAddressEditor()
}

private fun navigateToAddressEditor() {
private fun navigateToAddressEditor(address: Address? = null) {
navController.navigate(
AddressManagementFragmentDirections
.actionAddressManagementFragmentToAddressEditorFragment()
.actionAddressManagementFragmentToAddressEditorFragment(
address = address
)
)
}
}
25 changes: 25 additions & 0 deletions app/src/main/java/org/mozilla/fenix/settings/address/interactor/AddressEditorInteractor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package org.mozilla.fenix.settings.address.interactor

import mozilla.components.concept.storage.Address
import mozilla.components.concept.storage.UpdatableAddressFields
import org.mozilla.fenix.settings.address.controller.AddressEditorController

Expand All @@ -25,6 +26,22 @@ interface AddressEditorInteractor {
* @param addressFields A [UpdatableAddressFields] record to add.
*/
fun onSaveAddress(addressFields: UpdatableAddressFields)

/**
* Deletes the provided address from the autofill storage. Called when a user
* taps on the save menu item or "Save" button.
*
* @param guid The unique identifier for the [Address] record to delete.
*/
fun onDeleteAddress(guid: String)

/**
* Updates the provided address in the autofill storage. Called when a user
* taps on the update menu item or "Update" button.
*
* @param addressFields A [UpdatableAddressFields] record to add.
*/
fun onUpdateAddress(guid: String, addressFields: UpdatableAddressFields)
}

/**
Expand All @@ -44,4 +61,12 @@ class DefaultAddressEditorInteractor(
override fun onSaveAddress(addressFields: UpdatableAddressFields) {
controller.handleSaveAddress(addressFields)
}

override fun onDeleteAddress(guid: String) {
controller.handleDeleteAddress(guid)
}

override fun onUpdateAddress(guid: String, addressFields: UpdatableAddressFields) {
controller.handleUpdateAddress(guid, addressFields)
}
}
77 changes: 61 additions & 16 deletions app/src/main/java/org/mozilla/fenix/settings/address/view/AddressEditorView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,15 @@

package org.mozilla.fenix.settings.address.view

import android.content.Context
import android.content.DialogInterface
import androidx.appcompat.app.AlertDialog
import androidx.core.view.isVisible
import mozilla.components.concept.storage.Address
import mozilla.components.concept.storage.UpdatableAddressFields
import mozilla.components.support.ktx.android.view.hideKeyboard
import mozilla.components.support.ktx.android.view.showKeyboard
import org.mozilla.fenix.R
import org.mozilla.fenix.databinding.FragmentAddressEditorBinding
import org.mozilla.fenix.ext.placeCursorAtEnd
import org.mozilla.fenix.settings.address.interactor.AddressEditorInteractor
Expand All @@ -16,7 +22,8 @@ import org.mozilla.fenix.settings.address.interactor.AddressEditorInteractor
*/
class AddressEditorView(
private val binding: FragmentAddressEditorBinding,
private val interactor: AddressEditorInteractor
private val interactor: AddressEditorInteractor,
private val address: Address? = null
) {

/**
Expand All @@ -36,26 +43,64 @@ class AddressEditorView(
binding.saveButton.setOnClickListener {
saveAddress()
}

address?.let { address ->
binding.emailInput.setText(address.email)
binding.phoneInput.setText(address.tel)

binding.firstNameInput.setText(address.givenName)
binding.middleNameInput.setText(address.additionalName)
binding.lastNameInput.setText(address.familyName)

binding.streetAddressInput.setText(address.streetAddress)
binding.cityInput.setText(address.addressLevel2)
binding.stateInput.setText(address.addressLevel1)
binding.zipInput.setText(address.postalCode)

binding.deleteButton.apply {
isVisible = true
setOnClickListener { view ->
showConfirmDeleteAddressDialog(view.context, address.guid)
}
}
}
}

internal fun saveAddress() {
binding.root.hideKeyboard()

interactor.onSaveAddress(
UpdatableAddressFields(
givenName = binding.firstNameInput.text.toString(),
additionalName = binding.middleNameInput.text.toString(),
familyName = binding.lastNameInput.text.toString(),
organization = "",
streetAddress = binding.streetAddressInput.text.toString(),
addressLevel3 = "",
addressLevel2 = "",
addressLevel1 = "",
postalCode = binding.zipInput.text.toString(),
country = "",
tel = binding.phoneInput.text.toString(),
email = binding.emailInput.text.toString()
)
val addressFields = UpdatableAddressFields(
givenName = binding.firstNameInput.text.toString(),
additionalName = binding.middleNameInput.text.toString(),
familyName = binding.lastNameInput.text.toString(),
organization = "",
streetAddress = binding.streetAddressInput.text.toString(),
addressLevel3 = "",
addressLevel2 = "",
addressLevel1 = "",
postalCode = binding.zipInput.text.toString(),
country = "",
tel = binding.phoneInput.text.toString(),
email = binding.emailInput.text.toString()
)

if (address != null) {
interactor.onUpdateAddress(address.guid, addressFields)
} else {
interactor.onSaveAddress(addressFields)
}
}

internal fun showConfirmDeleteAddressDialog(context: Context, guid: String) {
Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure what to think about this. Instinctively, I feel like this should be called through the usual interactor/controller, but I don't know if I have a strong opinion about 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 see the pair interactor/controller usage as a bridge between the actions in the interface and the store and sometimes state. Showing a UI piece (a confirmation dialogue in this case) should be the responsibility of the fragment/view IMO. This would also allow for additional testing of the dialog behavior in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I generally agree with your stance. Where I would say I differ or don't hold a strong opinion on is the exact scope of what should be handled in the controller - I think handling all Store interactions in there makes sense, but otherwise, I would be looking to extract as much functionality out of the Fragment to make functionality easier to tests. Otherwise, I would say the Interactor is called for all user interactions, which is why I am on the fence since this is technically called through a user tapping on the "Delete button", but it's also called through the menu options.

The View in this case does satisfy the fact that we aren't putting the dialog responsibility in the fragment and thus making it easier to test. So, I could've gone either direction.

AlertDialog.Builder(context).apply {
setMessage(R.string.addressess_confirm_dialog_message)
setNegativeButton(R.string.addressess_confirm_dialog_cancel_button) { dialog: DialogInterface, _ ->
dialog.cancel()
}
setPositiveButton(R.string.addressess_confirm_dialog_ok_button) { _, _ ->
interactor.onDeleteAddress(guid)
}
create()
}.show()
}
}
20 changes: 20 additions & 0 deletions app/src/main/res/menu/address_editor.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- This Source Code Form is subject to the terms of the Mozilla Public
- License, v. 2.0. If a copy of the MPL was not distributed with this
- file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
<menu xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto">
<item
android:id="@+id/delete_address_button"
android:icon="@drawable/ic_delete"
android:title="@string/address_menu_delete_address"
android:visible="false"
app:iconTint="?attr/textPrimary"
app:showAsAction="ifRoom" />
<item
android:id="@+id/save_address_button"
android:icon="@drawable/mozac_ic_check"
android:title="@string/address_menu_save_address"
app:iconTint="?attr/textPrimary"
app:showAsAction="ifRoom" />
</menu>
8 changes: 7 additions & 1 deletion app/src/main/res/navigation/nav_graph.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,7 +1258,13 @@
<fragment
android:id="@+id/addressEditorFragment"
android:name="org.mozilla.fenix.settings.address.AddressEditorFragment"
android:label="@string/addresses_add_address" />
android:label="@string/addresses_add_address">
<argument
android:name="address"
android:defaultValue="@null"
app:argType="mozilla.components.concept.storage.Address"
app:nullable="true" />
</fragment>
<fragment
android:id="@+id/addressManagementFragment"
android:name="org.mozilla.fenix.settings.address.AddressManagementFragment"
Expand Down
12 changes: 12 additions & 0 deletions app/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1546,6 +1546,8 @@
<string name="credit_cards_biometric_prompt_unlock_message">Unlock to use stored credit card information</string>
<!-- Title of the "Add address" screen -->
<string name="addresses_add_address">Add address</string>
<!-- Title of the "Edit address" screen -->
<string name="addresses_edit_address">Edit address</string>
<!-- Title of the "Manage addresses" screen -->
<string name="addresses_manage_addresses">Manage addresses</string>
<!-- The header for the full name of an address -->
Expand Down Expand Up @@ -1574,6 +1576,16 @@
<string name="addresses_cancel_button">Cancel</string>
<!-- The text for the "Delete address" button for deleting an address -->
<string name="addressess_delete_address_button">Delete address</string>
<!-- The title for the "Delete address" confirmation dialog -->
<string name="addressess_confirm_dialog_message">Are you sure you want to delete this address?</string>
<!-- The text for the positive button on "Delete address" dialog -->
<string name="addressess_confirm_dialog_ok_button">Delete</string>
<!-- The text for the negative button on "Delete address" dialog -->
<string name="addressess_confirm_dialog_cancel_button">Cancel</string>
<!-- The text for the "Save address" menu item for saving an address -->
<string name="address_menu_save_address">Save address</string>
<!-- The text for the "Delete address" menu item for deleting an address -->
<string name="address_menu_delete_address">Delete address</string>

<!-- Title of the Add search engine screen -->
<string name="search_engine_add_custom_search_engine_title">Add search engine</string>
Expand Down
Loading