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

[$1000] Remove 'require cycles' developer warning message #14816

Closed
2 tasks done
Julesssss opened this issue Feb 3, 2023 · 35 comments
Closed
2 tasks done

[$1000] Remove 'require cycles' developer warning message #14816

Julesssss opened this issue Feb 3, 2023 · 35 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review

Comments

@Julesssss
Copy link
Contributor

Julesssss commented Feb 3, 2023

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


As pointed out here, a new developer warning message was introduced by this PR. It is not a regression, but should be fixed. I attempted to resolve this myself here, but it appeared to break notifications 😕

We need to correctly import the Push Notification library, making sure that notifications work in all cases:

  • App open
  • App backgrounded
  • App backgrounded with sign out and sign in with different account
  • App killed

Action Performed:

  1. As a developer, launch the app to an iOS or Android device/simulator
  2. Notice the following warning

Expected Result:

The warning should not show

Actual Result:

The warning shows

Workaround:

This is a developer-only issue, and it isn't a blocker

Platforms:

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

  • Android / native
  • iOS / native

Issue reported by: @dhairyasenjaliya

Simulator Screen Shot - iPhone 14 Pro Max - 2023-01-31 at 23 00 57
Simulator Screen Shot - iPhone 14 Pro Max - 2023-01-31 at 23 00 45

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019a3456a58e369fbd
  • Upwork Job ID: 1624093100668227584
  • Last Price Increase: 2023-02-10
@Julesssss Julesssss added the Bug Something is broken. Auto assigns a BugZero manager. label Feb 3, 2023
@Julesssss Julesssss self-assigned this Feb 3, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 3, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 3, 2023

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 3, 2023
@melvin-bot melvin-bot bot added the Daily KSv2 label Feb 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 3, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot unlocked this conversation Feb 3, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 3, 2023
@kevinksullivan
Copy link
Contributor

@Julesssss looks like this is iOS specific, yes? Also, I'm not sure what reproduction steps to take to complete the checklist. Can you help me understand the problem a bit more so I can reproduce? Thanks!

@melvin-bot melvin-bot bot added the Overdue label Feb 6, 2023
@Julesssss
Copy link
Contributor Author

Ah right, sorry I should have used the default template.

You're not going to be able to reproduce this as it's a developer environment issue only. It's occurring on Android and iOS and I'll update the description to make that clearer.

@melvin-bot melvin-bot bot removed the Overdue label Feb 6, 2023
@kevinksullivan
Copy link
Contributor

Thanks!

@Julesssss
Copy link
Contributor Author

@kevinksullivan updated, I think we can open it up

@melvin-bot melvin-bot bot added the Overdue label Feb 9, 2023
@Julesssss Julesssss added Improvement Item broken or needs improvement. Engineering External Added to denote the issue can be worked on by a contributor labels Feb 10, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 10, 2023
@melvin-bot melvin-bot bot changed the title Remove new developer warning message [$1000] Remove new developer warning message Feb 10, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~019a3456a58e369fbd

@MelvinBot
Copy link

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

@Julesssss Julesssss changed the title [$1000] Remove new developer warning message [$1000] Remove 'require cycles' developer warning message Feb 10, 2023
@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 10, 2023
@Julesssss
Copy link
Contributor Author

Julesssss commented Feb 13, 2023

Hey @abdulrahuman5196, that's interesting, thanks for taking a look.

I simply couldn't receive notifications when I attempted this, but perhaps this was due to another issue. I'm just going to test it once more to check.

@Julesssss
Copy link
Contributor Author

Julesssss commented Feb 13, 2023

Okay, well damn. My PR does work after all... Not sure what i messed up last time. I'm going to update it and put it in review.

@abdulrahuman5196 I'd be happy to recommend a 50% of bounty bonus for your review and the investigation you did. I'm curious how your PR differs from mine. Would you mind explaining by commenting on the PR?

@abdulrahuman5196
Copy link
Contributor

Okay, well damn. My PR does work after all... Not sure what i messed up last time. I'm going to update it and put it in review.

@abdulrahuman5196 I'd be happy to recommend a 50% of bounty bonus for your review and the investigation you did. I'm curious how your PR differs from mine. Would you mind explaining by commenting on the PR?

Wonderful !!!

I don't see major difference between your PR and what I tried other than "Only difference I see is we don't call init() when the UrbanAirship.getNamedUser() === accountID.toString() here - https://github.com/Expensify/App/blob/main/src/libs/Notification/PushNotification/index.native.js#L94. We can also return boolean here to determine whether to call init() or not in the session js file."

But I don't think this would be absolutely required, even without this change push notifications is working. Might have been some issue in debug key when you tried as I see now you are testing with release builds in PR? I tried with prod key as mentioned by you here - https://expensify.slack.com/archives/C01GTK53T8Q/p1675094829937589?thread_ts=1675092451.431619&cid=C01GTK53T8Q

@Julesssss Julesssss added Reviewing Has a PR in review and removed Reviewing Has a PR in review External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Feb 14, 2023
@Julesssss
Copy link
Contributor Author

Might have been some issue in debug key when you tried as I see now you are testing with release builds in PR? I tried with prod key as mentioned by you here

Yeah I think that's likely. PR is in review now.

@melvin-bot melvin-bot bot added the Overdue label Feb 16, 2023
@Julesssss Julesssss added the Reviewing Has a PR in review label Feb 17, 2023
@melvin-bot melvin-bot bot removed the Overdue label Feb 17, 2023
@Julesssss
Copy link
Contributor Author

PR merged and deployed.

@kevinksullivan could you please create an UpWork issue to pay @abdulrahuman5196 50% of the original bounty for his help please.

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Feb 22, 2023

Thank you @Julesssss

@kevinksullivan Applied in the upwork job melvin created here - #14816 (comment)

@kevinksullivan
Copy link
Contributor

Offer sent @abdulrahuman5196 . Let me know when you accept!

@dhairyasenjaliya
Copy link
Contributor

@kevinksullivan also can you invite me for reporting bug

@abdulrahuman5196
Copy link
Contributor

Offer sent @abdulrahuman5196 . Let me know when you accept!

Accepted the offer. @kevinksullivan

@kevinksullivan
Copy link
Contributor

Paid @abdulrahuman5196 . Offer sent @dhairyasenjaliya , let me know when you accept

@situchan
Copy link
Contributor

I am the first one who reported this regression here

@Julesssss
Copy link
Contributor Author

Sorry @situchan, I missed this. Lets make sure to report bugs in the Slack channel so that QA make the issue and correctly tag you next time.

@kevinksullivan would you mind adding the report payment for @situchan?

@dhairyasenjaliya
Copy link
Contributor

Hm if that's valid bug reporting before my posted on slack I would be happy to revert this milestone @Julesssss

@Julesssss
Copy link
Contributor Author

Honestly I'm struggling to see who posted if first. @dhairyasenjaliya I remember you posting in slack, that's why I created the issue, but I can't find the post and date anymore.

@dhairyasenjaliya
Copy link
Contributor

@Julesssss i think here i reported on slack https://expensify.slack.com/archives/C01GTK53T8Q/p1675186823689109?thread_ts=1675186823.689109&cid=C01GTK53T8Q

@situchan can you post your slack bug reporting link

@Julesssss
Copy link
Contributor Author

Julesssss commented Mar 3, 2023

Ah, I think it was reported first in that linked Github issue. I kinda think that the Slack report is the first official report, as that's where QA will be looking. Though there was only about an hour or two in between.

What do you think @kevinksullivan

@dhairyasenjaliya
Copy link
Contributor

Hm yeah I see that but if that bug report posted like that on github no external dev or internal dev can see if bug reported or not @Julesssss

@situchan
Copy link
Contributor

situchan commented Mar 3, 2023

I thought it was quicker to report on GH which caused regression so that PR author and reviewers know and quickly fix it before going to production. Slack bug reports usually take time for GH to be created, BZ assigned and engineer assigned and find offending PR.
From next time, I will officially report to slack channel and tag assignees with linked GH.
I am happy with team's decision.

@Julesssss
Copy link
Contributor Author

Thanks for understanding @situchan. I appreciate the idea behind commenting on the issue, but we'd definitely prefer it to be raised additionally in Slack.

I re-read our contribution guidelines and I'll try to make this clearer so we don't run into the same problem again.

@kevinksullivan
Copy link
Contributor

Just catching up from OOO, I agree with the approach you proposed @Julesssss . Thanks for updating the documentation as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants