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 "Update Pledge" option on Manage Your Pledge menu #2180

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jlplks
Copy link
Contributor

@jlplks jlplks commented Nov 28, 2024

📲 What

Remove "Update Pledge" option on Manage Your Pledge menu

🤔 Why

If plot is selected we should remove all the options that interact with UpdateBacking mutation

🛠 How

Created another xlm menu and added a validation for show it or not

@@ -1057,9 +1057,13 @@ interface ProjectPageViewModel {
private fun managePledgeMenu(projectAndFragmentStackCount: Pair<Project, Int>): Int {
val project = projectAndFragmentStackCount.first
val count = projectAndFragmentStackCount.second
// Check the feature flag
// TODO: Add another validation when API is ready to add the value that PLOT is selected on the project
val isPledgeOverTimeEnabled = featureFlagClient.getBoolean(FlagKey.ANDROID_PLEDGE_OVER_TIME)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jlplks you need to check as well on project, the new field to be added on graphQL api project.isPledgeOverTimeAllowed = true. Add in the todo which Jira ticket will be worked in at least if you wanna tackle this piece of the epic now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding it to Project data model

@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<menu xmlns:app="http://schemas.android.com/apk/res-auto"
Copy link
Contributor

Choose a reason for hiding this comment

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

@jlplks there is a menu already, I much likely prefer to identify where that menu is, and hide the options not available rather than creating a new one

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 way the menu options are managed inside the class is rendering differents xmls depending on the conditions, that's the reason for managePledgeMenu. That's why I opted for that way, do you prefer that I did it in another way?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jlplks

depending on the conditions
Those conditions are based on pledge status, when for this specific case you are just checking if the project has plot enabled, but not if the backing itself has been pledged to WITH plot.

your condition needs to change to something like:
val isPledgeOverTimeEnabled = featureFlagClient.getBoolean(FlagKey.ANDROID_PLEDGE_OVER_TIME) && project.isPledgeOverTimeAllowed() == true && backing.incremental == true

Screenshot 2024-12-02 at 1 12 49 PM

Otherwise you are adding this menu file to projects with plot enabled weather the user pledged with or without plot.

So you either update the menu for manage_pledge_life programatically depending on the backing information or add to your condition the check for the backing information.

Also remember to do the tests any time a VM is updated with new use cases/logic
Screenshot 2024-12-02 at 1 17 57 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added incremental to backing and added a test

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 85.20710% with 25 lines in your changes missing coverage. Please review.

Project coverage is 68.73%. Comparing base (2b898d9) to head (1e6c29f).

Files with missing lines Patch % Lines
...ter/viewmodels/projectpage/ProjectPageViewModel.kt 85.18% 14 Missing and 10 partials ⚠️
...pp/src/main/java/com/kickstarter/models/Backing.kt 85.71% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2180      +/-   ##
============================================
+ Coverage     68.66%   68.73%   +0.07%     
- Complexity     2168     2169       +1     
============================================
  Files           348      348              
  Lines         23186    23274      +88     
  Branches       3391     3397       +6     
============================================
+ Hits          15920    15997      +77     
- Misses         5453     5462       +9     
- Partials       1813     1815       +2     

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

Arkariang
Arkariang previously approved these changes Dec 2, 2024
Copy link
Contributor

@Arkariang Arkariang left a comment

Choose a reason for hiding this comment

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

👍 looks, good, this code can be merged, but wondering what tickets on the Jira epic will contain the connexion to the API where those two new fields (the one in Project, and the one in Backing ) are queried. Both fields are currently available on the API as shown on the screenshot comment

@jlplks
Copy link
Contributor Author

jlplks commented Dec 3, 2024

👍 looks, good, this code can be merged, but wondering what tickets on the Jira epic will contain the connexion to the API where those two new fields (the one in Project, and the one in Backing ) are queried. Both fields are currently available on the API as shown on the screenshot comment

I have 3 task for the API, I created them at the start of the PLOT I'll do it there, I will modify the task description to add that changes

@jlplks jlplks requested a review from Arkariang December 3, 2024 16:59
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.

3 participants