Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.87k forks source link

[HOLD for payment 2023-04-03] Unwanted double quotes in notification after ... for bigger messages #14849

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open the app and login with user A
  2. Open the app in android app and login with user B
  3. Send long message from user A to user B

Expected Result:

Larger notifcations should end with ... in the end

Actual Result:

Larger notification ends with ..." in the end (App is adding unwanted double quotes)

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.2.65-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/216880746-1c0aa913-58e3-4fa1-a43b-8238c1eb6877.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675500083078039

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01441300c2ff47e240
  • Upwork Job ID: 1624093978140704768
  • Last Price Increase: 2023-02-17
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

zanyrenney commented 1 year ago

Need to try and reproduce! Haven't been able to so far

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01441300c2ff47e240

MelvinBot commented 1 year ago

Current assignee @zanyrenney is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

MelvinBot commented 1 year ago

Triggered auto assignment to @johnmlee101 (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

alexxxwork commented 1 year ago

I've posted a Proposal for similar problem. To say short I suppose this logic is on the airship service side https://github.com/Expensify/App/issues/14715#issuecomment-1414433837

parasharrajat commented 1 year ago

@alexxxwork That can not be counted as a proposal as it does not propose any solutions with the analysis.

alexxxwork commented 1 year ago

@alexxxwork That can not be counted as a proposal as it does not propose any solutions with the analysis.

@parasharrajat Yeah, of course. But I think the most right thing to do is to address the problem to airship service. It would be a hard task to write rules for notification messages truncation on app side without thorough knowledge of how this work inside the airship service.

parasharrajat commented 1 year ago

Ok, I tried to find the source code related to Notifications but no luck. I am not an Android guy so don't know where to look exactly. Any help will be appreciated.

Yeah, of course. But I think the most right thing to do is to address the problem to airship service

Ok, @alexxxwork. IMO, Maybe this can easily be resolved from the app side with the proper configuration of the notification builder. I don't think we need to know anything about airship service. the Logic should already be there in the source.

What do you propose? How can you help to advance this issue?

parasharrajat commented 1 year ago

Waiting for proposals.

alexxxwork commented 1 year ago

Ok, @alexxxwork. IMO, Maybe this can easily be resolved from the app side with the proper configuration of the notification builder. I don't think we need to know anything about airship service. the Logic should already be there in the source.

@parasharrajat ok, we could go this way - we should count symbols on our side and truncate string sent to notification service. I'll write down a proposal by template.

zanyrenney commented 1 year ago

thanks for communicating @parasharrajat - still waiting for proposals here!

alexxxwork commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

We are trying to resolve an issue in long string coming to notifications on android native. Strings more than 115 ascii symbols get truncated with ...'' in the end.

What is the root cause of that problem?

The root cause of the problem is limits of the airship service for android plaform doc

What changes do you think we should make in order to solve the problem?

We should truncate string on our side here https://github.com/Expensify/App/blob/5a5b788afd91b765bab9d2dfd5de7d3468f0a216/android/app/src/main/java/com/expensify/chat/customairshipextender/CustomNotificationProvider.java#L84

What alternative solutions did you explore? (Optional)

We could address this issue upstream to airship

parasharrajat commented 1 year ago

Now sure, I understand the proposal.

Don't we have any API on the builder or something to customize the notification format? I like your optional solution. Do you want to raise it with them?

alexxxwork commented 1 year ago

Now sure, I understand the proposal.

  • How can you trim the message?
  • How will it solve the problem? What is the run-through?

Don't we have any API on the builder or something to customize the notification format? I like your optional solution. Do you want to raise it with them?

@parasharrajat We should count symbols in notification string taking in account that airship uses different encodings (we should analyze every symbol) and truncate it on our side in desired format so it doesn't truncate on airship side. I believe it's better to raise Airship attention on the problem because the above solution is only a workaround.

parasharrajat commented 1 year ago

Yes, I don't think this is the right way to trim the message. It might cause other issues and also what you are proposing has maintains cost to it.

Going to airship support is better.

alexxxwork commented 1 year ago

Yes, I don't think this is the right way to trim the message. It might cause other issues and also what you are proposing has maintains cost to it.

@parasharrajat So how should I address the issue to airship support? I suppose it's a commercial contract? https://www.airship.com/app-experience-platform/pricing/

parasharrajat commented 1 year ago

I think we can start with creating an issue on their repo.

alexxxwork commented 1 year ago

I think we can start with creating an issue on their repo.

@parasharrajat Airship react native readme (https://github.com/urbanairship/react-native-airship) says we should address issues to https://support.airship.com/. Should I make a service request?

parasharrajat commented 1 year ago

@alexxxwork I think you can open a ticket yourself. They might tell you the way to solve it. If they agree it is an internal problem and pushed a solution then we can hire you to merge the solution in our app.

MelvinBot commented 1 year ago

@johnmlee101 @parasharrajat @zanyrenney this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

johnmlee101 commented 1 year ago

Waiting for the ticket confirmation about the bug!

alexxxwork commented 1 year ago

Waiting for the ticket confirmation about the bug!

@johnmlee101 @parasharrajat https://support.airship.com/hc/en-us/requests/89947

MelvinBot commented 1 year ago

📣 @alexxxwork! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
alexxxwork commented 1 year ago

Contributor details Your Expensify account email: a@vcc.vc Upwork Profile Link: https://www.upwork.com/freelancers/~01558071651a8abfb5

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

parasharrajat commented 1 year ago

@alexxxwork Please share updates regularly on the progress of ticket.

parasharrajat commented 1 year ago

@zanyrenney @johnmlee101 Can anyone of you apply a hold on this issue? We are waiting on external factors so I don't think the price should increase unless there are more takers.

alexxxwork commented 1 year ago

@alexxxwork Please share updates regularly on the progress of ticket.

@parasharrajat I have provided requested additional info and they are investigating the issue.

parasharrajat commented 1 year ago

Still waiting on the support ticket.

fabioh8010 commented 1 year ago

Hi, I'm Fábio from Callstack and I would like to take a look at this issue.

MelvinBot commented 1 year ago

@johnmlee101, @parasharrajat, @zanyrenney Whoops! This issue is 2 days overdue. Let's get this updated quick!

parasharrajat commented 1 year ago

@alexxxwork any update. @fabioh8010 can you please post a plan you are going to take to solve it.

alexxxwork commented 1 year ago

@parasharrajat not much. I've provided some additional info on the ticket like AndroidManifest.xml, Airshipconfig.plist and so on. They asked to test push message truncation on their test app. Working on it now.

zanyrenney commented 1 year ago

Thanks for jumping in here @fabioh8010 !

fabioh8010 commented 1 year ago

Hi, @cead22 asked me to post my investigations about these notifications problems on this issue #14715. At the moment we are working together with Airship's team to address this problems.

parasharrajat commented 1 year ago

Ok. Thanks, both. @alexxxwork It seems we are taking these issues internally as it's been almost 3 weeks and we have not heard from Airship. We also agreed to compensate you for the same https://github.com/Expensify/App/issues/14715#issuecomment-1446606033 on another issue.

Thanks for your help.

@zanyrenney can you please apply the internal label and remove External and Help Wanted?

MelvinBot commented 1 year ago

Current assignee @parasharrajat is eligible for the Internal assigner, not assigning anyone new.

zanyrenney commented 1 year ago

hey @parasharrajat - that's done now!

zanyrenney commented 1 year ago

What is the latest here @johnmlee101 ?

Julesssss commented 1 year ago

I'm taking over as this is an internal server issue that I'm going to resolve as part of this wider task.

Julesssss commented 1 year ago

@zanyrenney while I have resolved the issue, I need a few more days to fix other issues that begin to occur once we remove the 115-char limit.

parasharrajat commented 1 year ago

Let me know if you need any help.

zanyrenney commented 1 year ago

Sounds good @Julesssss - thank you for the update!

alexxxwork commented 1 year ago

We also agreed to compensate you for the same #14715 (comment) on another issue.

@parasharrajat Could you please explain how this should work? Should I just wait for these issues to be resolved?

parasharrajat commented 1 year ago

Yes, once the linked issue is resolved, all compensations will be paid off.

zanyrenney commented 1 year ago

What's the latest here @Julesssss ?

Julesssss commented 1 year ago

Basically, this issue is already fixed, but I can't merge it yet.

Screenshot_20230313-154312

It modifies a lot of existing logic that handles notifications for OldDot, so I need to be very careful about the fix and rollout. I am migrating to a different method of building notifications, so we'll need to deploy the new method, but continue to be backward compatible for older users before making the final backend change. Also, I still need to fix the main issue (notifications being dropped when many emojis are in the message)

Maybe we should hold this one in the meantime to avoid needing daily updates -- because they won't really be related to THIS issue.

zanyrenney commented 1 year ago

Thanks for that!, So we're in a holding pattern here.