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

[Issue 3885] product variation delete confirmation dialog to a dialog fragment #10664

Merged
merged 11 commits into from
Feb 8, 2024

Conversation

jd-alexander
Copy link
Contributor

@jd-alexander jd-alexander commented Feb 1, 2024

Closes: #3885

Description

This PR was created to address the issue where the confirmation dialog is dismissed upon device rotation in the VariationDetailFragment, some notable changes:

  1. Reattaching the Listener: A method named reattachDialogInteractionListener was added to the VariationDetailFragment. This method is called within onViewCreated to re-establish the connection between the VariationDetailFragment and the WooDialogFragment. It checks if the dialog is currently displayed and, if so, sets the current instance of VariationDetailFragment as the listener to the dialog. This ensures that the dialog can continue to communicate with the active instance of the fragment after a configuration change.

  2. Event Handling: The ViewModel was updated to handle the delete action through a ShowDialogFragment event. This event encapsulates all necessary information to display the dialog, including message IDs and button texts. When the event is triggered, the dialog is shown with the current context, and any actions taken in the dialog are communicated back to the ViewModel through the established listener interface.

  3. Observing Lifecycle Events: The VariationDetailFragment observes the ShowDialogFragment event. If the fragment is recreated due to an orientation change, the observer ensures that the dialog is shown again with the correct listener attached.

Testing instructions

  1. Click a production with multiple variations.
  2. Click Variations.
  3. Open a variation.
  4. Click the menu option.
  5. Press Delete.
  6. See the dialog visible.
  7. Rotate the device and realize that the dialog is still open.
  8. Verify that the actions on the dialog work as expected.

Images/gif

Screen_Recording_20240131_175116_WooCommerce.Dev.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@jd-alexander jd-alexander added this to the 17.3 milestone Feb 1, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Feb 1, 2024

2 Warnings
⚠️ Class ShowDialogFragment is missing tests, but unit-tests-exemption label was set to ignore this.
⚠️ This PR is assigned to the milestone 17.3. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Feb 1, 2024

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
FlavorJalapeno
Build TypeDebug
Commit2b41899
Direct Downloadwoocommerce-prototype-build-pr10664-2b41899.apk

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (92810a2) 41.47% compared to head (18094aa) 41.56%.

❗ Current head 18094aa differs from pull request most recent head 2b41899. Consider uploading reports for the commit 2b41899 to get more accurate results

Files Patch % Lines
.../com/woocommerce/android/ui/dialog/DialogParams.kt 0.00% 9 Missing ⚠️
...ui/products/variations/VariationDetailViewModel.kt 0.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #10664      +/-   ##
============================================
+ Coverage     41.47%   41.56%   +0.09%     
+ Complexity     5016     5009       -7     
============================================
  Files          1015     1011       -4     
  Lines         58299    58075     -224     
  Branches       7793     7735      -58     
============================================
- Hits          24178    24140      -38     
+ Misses        31977    31790     -187     
- Partials       2144     2145       +1     

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

@jd-alexander jd-alexander added the type: enhancement A request for an enhancement. label Feb 1, 2024
@peril-woocommerce
Copy link

peril-woocommerce bot commented Feb 1, 2024

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@jd-alexander
Copy link
Contributor Author

Hi @AnirudhBhat I've reviewed the PR requirements regarding Tracks-related updates. I want to clarify that my changes only involved moving existing Tracks logic to a new function, without altering its behavior or the events it triggers. Given this, I'm unsure if the full validation process in the Tracks system and updating the internal spreadsheet is necessary. Could you advise if these steps are still needed for such changes? Thanks for your guidance!

@jd-alexander jd-alexander marked this pull request as ready for review February 2, 2024 16:19
@AnirudhBhat AnirudhBhat self-assigned this Feb 7, 2024
Copy link
Contributor

@AnirudhBhat AnirudhBhat left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @jd-alexander. Works as expected! :shipit:

@jd-alexander jd-alexander merged commit f52dff3 into trunk Feb 8, 2024
15 checks passed
@jd-alexander jd-alexander deleted the issue-3885/variation-delete-confirmation branch February 8, 2024 01:02
@AnirudhBhat
Copy link
Contributor

Given this, I'm unsure if the full validation process in the Tracks system and updating the internal spreadsheet is necessary. Could you advise if these steps are still needed for such changes?

Sorry, I missed your comment. This PR does not require any modifications to the spreadsheet since it only involves reorganizing existing events, without adding or removing any.

@jd-alexander
Copy link
Contributor Author

Given this, I'm unsure if the full validation process in the Tracks system and updating the internal spreadsheet is necessary. Could you advise if these steps are still needed for such changes?

Sorry, I missed your comment. This PR does not require any modifications to the spreadsheet since it only involves reorganizing existing events, without adding or removing any.

No problem! Thanks for chiming in. I appreciate it @AnirudhBhat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A request for an enhancement. unit-tests-exemption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Variations] Delete confirmation dialog dismissed when the device orientation changes
5 participants