Supporting the new Android plugins APIs & compatibility with flutter 3 #62
Conversation
- updating version number - fixing example to work with api 31
GJJ2019
changed the title
|
Thanks for the PR @reeteshranjan and I will take a look shortly |
| val icon = if (bitmap != null) { | ||
| encodeToBase64(bitmap) | ||
| } else { | ||
| null | ||
| } |
There was a problem hiding this comment.
Why was the null check guard removed?
There was a problem hiding this comment.
I'm not sure about this change will take a look maybe it got reformatted in Android studio
| // | ||
| // override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?): Boolean { | ||
| // if (requestCodeNumber == requestCode && result != null) { | ||
| // if (data != null) { | ||
| // try { | ||
| // val response = data.getStringExtra("response")!! | ||
| // this.success(response) | ||
| // } catch (ex: Exception) { | ||
| // this.success("invalid_response") | ||
| // } | ||
| // } else { | ||
| // this.success("user_cancelled") | ||
| // } | ||
| // } | ||
| // return true | ||
| // } |
There was a problem hiding this comment.
Go ahead and remove redundant code instead of commenting them out
There was a problem hiding this comment.
Yeah it was just for reference purposes 😂 I'll change this too my knowledge of kotlin is not upto the point
example/android/app/build.gradle
Outdated
| applicationId "com.drenther.upi_pay_example" | ||
| minSdkVersion 16 | ||
| targetSdkVersion 30 | ||
| applicationId "com.example.testing_plugin_example" |
There was a problem hiding this comment.
Please don't change things that don't need to be changed. Helps keeps the change surface area small
There was a problem hiding this comment.
Yes will change this too sorry about that
- fixing applicationId in example build.gradle
|
I have fixed those two unnecessary issues @drenther @reeteshranjan |
|
@GJJ2019 thank you so much for this awesome work. It seems the Intent related changes are finally done. I had got stuck there trying to make the code move to new Flutter abstractions and removing the ones marked obsolete. Could you please let us know what kind of testing have you done? When I expanded the package's support to multiple apps, there were some issues found and though the package itself was not at fault, it was crucial to add a workaround, which was neither violating nor breaking any standard. To ensure that everything works with such a major structural change, I would like to see that several UPI apps, say around 5 major and 5 minor, are verified to work fine i.e. a) payment goes through, b) payment response is collected and can be printed or shown in an alert box, c) unsuccessful payment responses are collected and can be printed or shown in an alert box. |
|
Ok @reeteshranjan will try to do this stuff asap |
|
Tried using this for my production app, but I am not getting callbacks of success/failures after the payment. |
|
HI @mrinaljain, i have added adding onActivityResult callback on my recent commit can u check that if its working or not. |
|
My upi account is not working it seems so I'm not able to test properly @reeteshranjan |
What is the error? Is it specific to a particular app? Which apps have you tried to test? |
|
Im not sure but i have raised complaint about it my bank will proceed, its something to do with my bank |
|
I'm sure plugin will work just fine but we need to test it properly. I'm not able to do that |
HI @GJJ2019 I JUST CHECKED AGAIN , still I am not receiving any callbacks for success or failure. Basically I want to await for the upi transaction result
|
|
any updates on this one ? @GJJ2019 |
|
I'm not sure will check this week & fix, test all the issues for sure :) |
|
I have fixed that activity result not getting called functionality can u test the payment @mrinaljain |
|
@drenther @reeteshranjan @mrinaljain Please review it, the issue that this PR is fixing is important. |
|
yes but can u also provide test result so that its easy to merge the code @MDSADABWASIM |
With these changes, I can build successfully, but payment is failing for me, this is the first time I'm integrating this plugin so don't know if the issue is caused by changes or my implementation @GJJ2019 |
|
I'll test this plugin over the weekend, and update here! |
@MDSADABWASIM what is the error you are getting? is it failing a) on getting app link b) invoking the app from app list, c) after going inside the upi app? Also is it possible to share error message ? if your answer is 'b' can you try with : https://github.com/gururaja-kambala/upi_pay.git |
|
Hi @GJJ2019 I'm following this PR since last week for the Only safe (?.) or non-null asserted (!!.) calls are allowed on a nullable receiver of type Activity? error, and it seems this is fixed now in the above PR's. Can you please quickly help me which version I need to upgrade in pubsec.yaml file.. Currently I'm using upi_pay: ^1.0.1 and seeing the above error. By scrolling through other PR's I could see somewhere version 2.0.0 but that version seems to be not exist/public. |
|
@drenther is right man to ask |
|
I think activity will get initialise when flutter engine fires so mostly it will be non nullable without some edge cases |
@drenther - Can you please help here a bit, it will be highly appreciated. |
|
@crossxsecure I have created another plugin for easy_upi_payment |
Highly appreciated your efforts @GJJ2019, my core requirement is I need it for iOS and I believe this is the only plugin available for it? Moreover, I went through your plugin and that's really a great offering, but I feel you should also include a optional bundleName/packageName in the input parameter so that it can be used utilized as per the custom needs as well. Apart from that it's awesome and a big thanks to all plugin builders. |
version 2.0.0 is this PR and since this PR is not merged yet you can understand why that is not published on pub.dev |
|
@mrinaljain and anyone else who were looking into testing this PR, could you please provide any update on the testing? |
Is there any place I can get details of those cases? is it on return status post transaction? or before invoking app? Is it because, it is not plugin will not be compatible with lower version of flutter, once we handle changes required for flutter 3.0? |
@reeteshranjan I tested the updated PR and It's working fine. Basically, I wanted to await the UPI transaction result which was previously not working. but now I am able to use the code below
|
Thanks! I will verify with my setup of multiple apps on Android and iOS. |
|
@reeteshranjan Any updates on this PR , when can we start using for production build ? |
Sorry have been very busy. No bandwidth. Hopefully, other devs can pitch in for testing. |
|
I have re-asked my questions to NPCI about #38 . If on Twitter, please do look at https://twitter.com/reeteshr08/status/1561430970930081793 and support. |
Testing UpdateUpdating results of few apps I have tested so far. Device DetailsDevice: Redmi Note 10 CodesUPI Setup Statuses (USS)
Transaction Functioning Status (TFS)
FindingsTested various apps. Depending on the findings of testing, a PR Health Indication is derived. It's one of:
Last date of testing PR: 24-Aug-2022 PR Health Indication: Needs Review
PR Health Indication: Cannot Say
PR Health Indication: OK
|
How to use the above updates?Testing focused on finding any cases where the updated code caused any new communication and response returning issues, which could imply regression in integration with payment apps. There are few such cases found, for which the PR Health Indication column has been marked as Needs Review. In several of these cases, there is a crash stack log also added. Initial look at these crashes points to some regression, to me. Need @GJJ2019 and other devs to take a better look. What's the current status of merging this PR?Should be done once all rows with PR Health Indication marked as Needs Review are resolved as not a problem with the new code. More testing?Several more apps need to be tested on Android and iOS. Will update here as more testing is done. However; given that we found some cases where there could be a regression, we should look at these cases and attempt resolving them first. |
|
I'm not sure about need review thing only thing i did was migrated api to v2 that's it |
Let's just take one case. Could you please install and configure Maha UPI (Bank of Maharashtra app), and reproduce the crash? It's related to intents. Exactly what should not be happening with any major upgrade where regression of working stuff happens. If we could determine that the crash is not because of the upgrade in our package; but due to some compatibility issue in the app we are trying to use, it's fine and we can move ahead with that properly marked. I'll update the APPS.md accordingly that a particular working app is no longer supported, and that's not due to what we have done. Point is maintaining the detailed status of support data for several BHIM UPI apps with utmost transparency, which is what my objective is with APPS.md. I definitely want at least @GJJ2019 and @drenther to look into my test results and come up with the calls/explanations. |
|
just one question @reeteshranjan are those same issues happening in previous version ? |
|
|
Updated the testing findings comment to organise the information better. @GJJ2019 |
|
@reeteshranjan When can I expect to merge this PR? |
|
@GJJ2019 still showing a deprecated issue, why is this pr still not merged, could anyone help? |
|
when are u going to release?? |
|
please merge the pr and release it. |
|
@nayanAubie @altafkhan8719 @grit-ameen please check the history to see that testing of the PR threw out few things that should be looked at before merging. |
|
This PR is not ready to be merged as @reeteshranjan has pointed out in details here Both @reeteshranjan and I do not currently have the capacity as of now to take this PR up and wrap it up. If any of you can, in this PR or a new one, contribute the code needed along with thorough test cases covered (in the lines of what @reeteshranjan has added in the repo's App support documentation page as well as the comment shared on this PR). We would be happy to review and merge. |
Changes in this PR:
Reference GitHub Issues:
#44 #60
Related Screenshots: