[HOLD for payment 2024-08-02] [$250] Tags - Settings button appears after delay after enabling Tags

izarutskaya commented 4 months 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!

Version Number: 1.4.72-0 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Launch New Expensify app.
Go to Account settings > Workspaces.
Go to workspace that has not enabled Tags.
Go to More features.
Enable Tags.
Go to Tags section.

Expected Result:

Settings button will appear immediately.

Actual Result:

Settings button appears after delay.

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/16034950-0891-441f-a2fe-743433f8fe4f

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013fd4c53afba54b03
Upwork Job ID: 1789468407388459008
Last Price Increase: 2024-05-19
Automatic offers:

nkdengineer | Contributor | 102448137

Issue Owner

Current Issue Owner: @alexpensify

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

izarutskaya commented 4 months ago

We think this issue might be related to the #collect project.

cretadn22 commented 4 months ago

Proposal

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

What is the root cause of that problem?

https://github.com/Expensify/App/blob/c19eea65c4688e8f353df6358cfd93761cea79bd/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L244-L250

In OPEN_POLICY_TAGS_PAGE command API, we don't set policyTags optimistic data. When enable Tag feature we need to wait BE return policyTags

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

We should establish default policy tags similar to the response from the backend in the optimistic data of the EnablePolicyTags command API.

{Tag: {name: "Tag", orderWeight: 0, required: false, tags: []}}

What alternative solutions did you explore? (Optional)

https://github.com/Expensify/App/blob/c19eea65c4688e8f353df6358cfd93761cea79bd/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L244-L250

Remove policyTag condition

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

melvin-bot[bot] commented 4 months ago

Job added to Upwork: https://www.upwork.com/jobs/~013fd4c53afba54b03

melvin-bot[bot] commented 4 months ago

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

ShridharGoel commented 4 months ago

Proposal

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

Settings button appears with delay on enabling tags.

What is the root cause of that problem?

Settings button would only show when policyTags data is available. policyTags data takes some time to be fetched via Onyx, which leads to the delay.

https://github.com/Expensify/App/blob/2a6d63d7f17b1e4fba94f26103f13c6afe294b91/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L248-L256

Also, the condition for isLoading is wrong, it just checks for undefined value of policyTags and not for null. Hence, the loading icon doesn't show while the tags are being fetched.

https://github.com/Expensify/App/blob/2a6d63d7f17b1e4fba94f26103f13c6afe294b91/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L162

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

Method 1:

Update the isLoading condition:

const isLoading = !isOffline && !policyTags;

We should be showing the settings option only when there's at least one tag/if the data is still loading (we can discuss if we want to hide the settings option while the data is loading, and then show it only if the there's at least one tag when the data has loaded).

{(tagList?.length > 0 ?? isLoading) && (
    <Button
        medium
        onPress={navigateToTagsSettings}
        icon={Expensicons.Gear}
        text={translate('common.settings')}
        style={[isSmallScreenWidth && styles.w50]}
    />
)}

We also need to update the width of "Add tag" button when settings button is not showing:

<Button
    medium
    success
    onPress={navigateToCreateTagPage}
    icon={Expensicons.Plus}
    text={translate('workspace.tags.addTag')}
    style={[styles.mr3, isSmallScreenWidth && (tagList?.length > 0 ? styles.w50 : styles.w100)]}
/>

Also, add this in AccessOrNotFoundWrapper in WorkspaceTagsSettingsPage to handle when link is directly opened:

shouldBeBlocked={(policyTagLists[0]?.tags ?? []).length === 0}

This would show not found page if there's no tag available and user tries to access tag settings page.

Method 2:

Use initialValue in Onyx and define it as an empty object. Then, while the data is being fetched, an empty object will be used as policyTags.

initialValue: {} as OnyxEntry<OnyxTypes.PolicyTagList>,

Optional:

We can change the navigation to featureRoute instead of initial workspace page:

if (isNarrowLayout) {
    setTimeout(() => {
        Navigation.navigate(featureRoute);
    }, 1000);
    return;
}

We are navigating to featureRoute for bigger screens as well in the existing flow.

We can discuss if we want to update the timeout as well, else if 1000 ms is fine then we can keep it the same.

nkdengineer commented 4 months ago

Proposal

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

Settings button appears after delay.

What is the root cause of that problem?

At the first time user turn on the "Tags" feature, it needs the time to retrieve the policyTags_ data. And when the policyTags_ is loading, the policyTags in here is null, leads to the Settings button in here is not displayed.

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

I think we do not need to add the condition when displaying the Settings button because: Clicking on the Settings button will open WorkspaceTagsSettingsPage page. But we can also open this page using the deeplink. That means although we try to prevent user from clicking on Settings button ( to prevent them from accessing the WorkspaceTagsSettingsPage as well), they still can access WorkspaceTagsSettingsPagein other ways without any condition.

To do it:

So we just need to remove the policyTags condition in here.

Then, we can display the loading indicator in WorkspaceTagsSettingsPage if the policyTags_ data is not avaiable. For example, we can add the below to WorkspaceTagsSettingsPage:

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

with isLoading: !PolicyUtils.getTagLists(policyTags)?.[0]

In case of offline mode, we can display the offline page.

What alternative solutions did you explore? (Optional)

In 2. in main solution, we can disable (or hide) Custom tag name option if isLoading is true instead

nkdengineer commented 4 months ago

I updated proposal

sobitneupane commented 4 months ago

Thanks for the proposal @cretadn22 .

I don't think we can optimistically assume {Tag: {name: "Tag", orderWeight: 0, required: false, tags: []}} to be policyTags. User can update the tag name and disable Tags and reenable it. In that case, server will return the updated tag name.

sobitneupane commented 4 months ago

Thanks for the proposal @ShridharGoel

I am not sure if we want to hide Settings if there are no tags. As of now, we are only hiding Settings if policyTags is not loaded yet. Should we just consider policyTags to determine the Settings page?

sobitneupane commented 4 months ago

Thanks for the proposal @nkdengineer

But we can also open this page using the deeplink. That means although we try to prevent user from clicking on Settings button ( to prevent them from accessing the WorkspaceTagsSettingsPage as well), they still can access WorkspaceTagsSettingsPagein other ways without any condition.

Thanks for bringing this up.

Do we have any other such instances in the app where we use Loading Indicator in RHN? I only have suspicion on how will it behave when user is offline. Should we display offline page if user if offline and the required data is not available?

nkdengineer commented 4 months ago

@sobitneupane You can try to open the note page in offline, the loading indicator is displayed when we are offline. URL example: https://dev.new.expensify.com:8082/r/4122908816096140/notes/16802645/edit.

cretadn22 commented 4 months ago

@sobitneupane Updated proposal We ought to establish a default policyTag within the optimistic data of the EnablePolicyTags API command. This approach mirrors how we implemented it in the enablePolicyTaxes function. https://github.com/Expensify/App/blob/c19eea65c4688e8f353df6358cfd93761cea79bd/src/libs/actions/Policy.ts#L4203

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

sobitneupane commented 4 months ago

Thanks for the update everyone.

In my opinion, it would be more consistent with the app's design to display the Settings button and show a loading indicator when the Settings button is clicked if the data is not yet available.

Proposal from @nkdengineer looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 4 months ago

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

MitchExpensify commented 4 months ago

@MonilBhavsar does @nkdengineer 's proposal look good to you too? Let's assign if so 🙇

melvin-bot[bot] commented 3 months ago

@sobitneupane @MonilBhavsar @MitchExpensify 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!

melvin-bot[bot] commented 3 months ago

@sobitneupane, @MonilBhavsar, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

MonilBhavsar commented 3 months ago

Sorry, since this is owned by C+. I missed it in my K2.

The proposal looks good mostly. I have one concern - how does it work in offline mode? Are we displaying an infinite loader? And as mentioned above, if user is able to access the settings page using deep-link then displaying infinite loader on button does not sound right. A POC could help in this case. Thanks!

sobitneupane commented 3 months ago

@MonilBhavsar

how does it work in offline mode? Are we displaying an infinite loader?

The plan is not to show a loader on the button. Instead, we will display the Settings button regardless of whether data is available. When the user clicks the Settings button, the RHP will open. If data is available, we will show the details; otherwise, we will show the loader.

And as mentioned above, if user is able to access the settings page using deep-link then displaying infinite loader on button does not sound right.

If the user doesn't have access to the settings page, we will show a 'Not Found' page on the RHP. If the user has access to the settings page but the data has not been fetched yet, we will show a loading indicator on the RHP.

@nkdengineer Can you please confirm the details?

nkdengineer commented 3 months ago

@MonilBhavsar The solution is what @sobitneupane said above. The similar logic is the note page. You can try to open the note page in offline, the loading indicator is displayed when we are offline. URL example: https://dev.new.expensify.com:8082/r/4122908816096140/notes/16802645/edit.

ShridharGoel commented 3 months ago

@MonilBhavsar @sobitneupane But why should we even show the settings option if policyTags data is not available? Can you check my proposal once?

MonilBhavsar commented 3 months ago

Thanks for the link and context 🙌

I see in the edit notes page in offline mode, we're displaying infinite loader. That seems confusing UX because loading indicates that we're trying to load something, but infact we're not actually loading anything as user is offline. I think we should display offline page, indicating that user is offline and we're unable to load. Anything we do, we should also fix it at other places. If there are more pages, we can make a different issue. If there's single page, I believe we could do in this issue.

https://github.com/Expensify/App/assets/32012005/4e6971a1-10bf-4182-96ff-56a266fc17aa

MonilBhavsar commented 3 months ago

This falls under UX pattern D. Please see this flowchart https://github.com/Expensify/App/blob/main/contributingGuides/OFFLINE_UX.md#ux-pattern-flow-chart

nkdengineer commented 3 months ago

@MonilBhavsar I updated the proposal to handle the offline mode.

MonilBhavsar commented 3 months ago

But why should we even show the settings option if policyTags data is not available?

It does not follow the UI guidelines. We should not hide any button. We should handle the view triggered by the button accordingly, or we can disable the button when offline, if it makes network call. You can refer to this doc https://github.com/Expensify/App/blob/main/contributingGuides/OFFLINE_UX.md

MonilBhavsar commented 3 months ago

@nkdengineer thanks! Please add separate tests for Offline mode in the PR. And see if we can make a quick fix for notes page, while we're here. It can serve as a wrong inspiration for offline UX, like we did here.

melvin-bot[bot] commented 3 months ago

📣 @nkdengineer 🎉 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 📖

nkdengineer commented 3 months ago

@MonilBhavsar Do we have the design for the offline screen? Here is the offline screen in the attachment:

MonilBhavsar commented 3 months ago

Yes, it's this and component is FullPageOfflineBlockingView

sobitneupane commented 3 months ago

@nkdengineer When can we expect the PR?

nkdengineer commented 3 months ago

@sobitneupane PR https://github.com/Expensify/App/pull/42623 is ready

melvin-bot[bot] commented 3 months ago

This issue has not been updated in over 15 days. @sobitneupane, @MonilBhavsar, @MitchExpensify, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

MitchExpensify commented 2 months ago

PR is actively being worked on, looks like it needs your review as a next step @sobitneupane

Reassiging as I'm about to head on paternity leave

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

alexpensify commented 2 months ago

This is noted. I'm taking over and looking at the PR. @nkdengineer, there is some feedback that needs your attention. Thanks!

alexpensify commented 1 month ago

Thanks, the PR is moving along again through feedback.

alexpensify commented 1 month ago

Awesome, this one is on staging now and we will wait for automation to run.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/42623

If no regressions arise, payment will be issued on 2024-08-02. :confetti_ball:

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

@sobitneupane requires payment through NewDot Manual Requests
@nkdengineer requires payment automatic offer (Contributor)

melvin-bot[bot] commented 1 month ago

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:

[ ] [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@sobitneupane] 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:
[ ] [@sobitneupane] 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:
[ ] [@sobitneupane] Determine if we should create a regression test for this bug.
[ ] [@sobitneupane] 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 commented 1 month ago

Payouts due: 2024-08-02

[x] Contributor: $250 @nkdengineer
[ ] Contributor+: $250 @sobitneupane - please submit a request in Chat, thanks!

Upwork job is here.

garrettmknight commented 2 weeks ago

$250 approved for @sobitneupane based on this summary.

Expensify / App