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

[HOLD for payment 2024-04-05] [$125] Workspace - Categories load without loading spinner #37904

Closed
6 tasks done
izarutskaya opened this issue Mar 7, 2024 · 42 comments
Closed
6 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Mar 7, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Found when validating PR : #37457

Version Number: v1.4.48-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Pre-requisite: an admin account with a policyExpenseChat enabled Collect policy and another workspace.

  1. Open the app
  2. Login with admin account with Collect policy
  3. Navigate to Workspace > Settings and select the collect policy.
  4. Select the Categories page
  5. Verify that you see the loading spinner

Expected Result:

Categories load with loading spinner

Actual Result:

Categories load without loading spinner

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

2024-03-07.16.27.56.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01197fc8d66a9362f0
  • Upwork Job ID: 1770919042911531008
  • Last Price Increase: 2024-03-21
  • Automatic offers:
    • DylanDylann | Reviewer | 0
    • GandalfGwaihir | Contributor | 0
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 7, 2024
Copy link

melvin-bot bot commented Mar 7, 2024

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

@izarutskaya
Copy link
Author

@alexpensify I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@izarutskaya
Copy link
Author

We think that this bug might be related to #wave8-collect-admins
CC @zanyrenney

@neonbhai
Copy link
Contributor

neonbhai commented Mar 7, 2024

Proposal

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

Categories load without loading spinner

What is the root cause of that problem?

We have loading spinner logic on the page, but we only check if the policyCategories are undefined, but these can be null, before fetching. Since isLoading isn't true when it is null. No loading spinner is shown

{isLoading && (
<ActivityIndicator
size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE}
style={[styles.flex1]}
color={theme.spinner}
/>
)}

const isLoading = !isOffline && policyCategories === undefined;

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

We should add a check for if policyCategories are null here

const isLoading = !isOffline && policyCategories === undefined;

@allgandalf
Copy link
Contributor

Proposal

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

Categories load without loading spinner

What is the root cause of that problem?

We have !isLoading set for the state where the categories are empty:

{categoryList.length === 0 && !isLoading && (
<WorkspaceEmptyStateSection

But we don't have a !isLoading state for when we list of categories:

{categoryList.length > 0 && (
<SelectionList

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

We need to add a !isLoading parameter to the list:

{categoryList.length > 0 && !isLoading && (
                        <SelectionList

What alternative solutions did you explore? (Optional)

N/A

@alexpensify
Copy link
Contributor

I'll test early next week and we can figure out the next steps. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

@alexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@alexpensify
Copy link
Contributor

No update yet, I need to test.

@melvin-bot melvin-bot bot removed the Overdue label Mar 11, 2024
@allgandalf
Copy link
Contributor

bump @alexpensify for review

@alexpensify
Copy link
Contributor

I've started a discussion in the wave room related to Collect - https://expensify.slack.com/archives/C036QM0SLJK/p1710357619580229. I should have an update by tomorrow.

@luacmartins
Copy link
Contributor

This is not a bug. We only want to show the loading spinner if we have no local data.

@luacmartins
Copy link
Contributor

Closing this issue.

@alexpensify
Copy link
Contributor

Thanks @luacmartins for the update!

@allgandalf
Copy link
Contributor

Actually we do need to show a spinner cause when we make a collect workspace for the first there is no local data and we tend to see only the header like name and status at bottom of page without any present data, so i guess we should enable a loading spinner mentioned in my proposal in the category list as well:

simplescreenrecorder-2024-03-20_19.35.25.mp4

Can we reopen this issue ? @alexpensify @luacmartins

@luacmartins
Copy link
Contributor

@GandalfGwaihir AFAIK we already do that here. Am I missing something?

@allgandalf
Copy link
Contributor

allgandalf commented Mar 20, 2024

Actually we need to add extra check over here for category list, currently:

{!shouldShowEmptyState && (
<SelectionList
canSelectMultiple
sections={[{data: categoryList, indexOffset: 0, isDisabled: false}]}

!shouldShowEmptyState is :

const shouldShowEmptyState = !categoryList.some((category) => category.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) && !isLoading;

here !isLoading is used.
So here when isLoading is true, essentially shouldShowEmptyState is set to false, but in category display, we have used !shouldShowEmptyState which will be true and we see the header preview as seen in the attached video,

We need to add an extra check:

{!shouldShowEmptyState && !isLoading &&  ( 
     <SelectionList 
         canSelectMultiple 
         sections={[{data: categoryList, indexOffset: 0, isDisabled: false}]} 

@allgandalf
Copy link
Contributor

we already do that here. Am I missing something?

Over there the isLoading check is only for the spinner icon and not the SelectionList

@allgandalf
Copy link
Contributor

friendly bump @luacmartins :)

@luacmartins
Copy link
Contributor

I think we should just mirror the conditions we have for the tags page. This was also what we initially had for categories, but we changed it somewhere along the way.

@allgandalf
Copy link
Contributor

allgandalf commented Mar 21, 2024

Makes sense :) Are we reopening this issue then ?

@DylanDylann
Copy link
Contributor

DylanDylann commented Mar 22, 2024

This is so straightforward. @GandalfGwaihir's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Mar 22, 2024

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@allgandalf
Copy link
Contributor

also @luacmartins as this is for tags pages as well, i guess the bounty can be $250 right :)

@DylanDylann
Copy link
Contributor

DylanDylann commented Mar 22, 2024

@GandalfGwaihir Please also check all other pages like tax page, member page, distance rate page,...

@allgandalf
Copy link
Contributor

allgandalf commented Mar 22, 2024

@GandalfGwaihir Please also check all other pages like tax page, member page, distance rate page,...

noted :)

c.c. @luacmartins

@luacmartins
Copy link
Contributor

luacmartins commented Mar 22, 2024

also @luacmartins as this is for tags pages as well, i guess the bounty can be $250 right :)

Sorry, the changes are really simple so we'll keep it at $125

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 22, 2024
Copy link

melvin-bot bot commented Mar 22, 2024

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Mar 22, 2024

📣 @GandalfGwaihir 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@allgandalf
Copy link
Contributor

Sorry, the changes are really simple so we'll keep it at $250

$125 right?

@luacmartins
Copy link
Contributor

Yes, sorry haven't fully woken up yet. $125 I edited my message above.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Mar 22, 2024
@allgandalf
Copy link
Contributor

PR ready for review @luacmartins @DylanDylann

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 29, 2024
@melvin-bot melvin-bot bot changed the title [$125] Workspace - Categories load without loading spinner [HOLD for payment 2024-04-05] [$125] Workspace - Categories load without loading spinner Mar 29, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 29, 2024
Copy link

melvin-bot bot commented Mar 29, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Mar 29, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.57-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-04-05. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Mar 29, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
  • [@DylanDylann] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@DylanDylann] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@DylanDylann] Determine if we should create a regression test for this bug.
  • [@DylanDylann] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@alexpensify
Copy link
Contributor

I confirmed the job link works here. I'll complete the payment process tomorrow.

https://www.upwork.com/jobs/~01197fc8d66a9362f0

@alexpensify
Copy link
Contributor

alexpensify commented Apr 5, 2024

Payouts due: 2024-04-05

  • Contributor: $125 @GandalfGwaihir
  • Contributor+: $125 @DylanDylann

Upwork job is here.


Closing - Everyone has been paid here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

6 participants