text:
Lerc:EncodeTempl missing from final .so

I think this PR by @tmaurer3  introduced a small API issue: https://github.com/Esri/lerc/pull/147
By introducing the EncodeInternal template function, the compiler now fails to include exported versions of either Lerc::EncodeTempl or Lerc:: ComputeCompressedSizeTempl.
$ nm libLerc.so  | c++filt | grep "Lerc::Encode"
0000000000016260 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<signed char>(signed char const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
00000000000190b0 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<double>(double const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
00000000000185a0 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<float>(float const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
0000000000016700 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<unsigned char>(unsigned char const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
0000000000017a80 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<int>(int const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
0000000000017f20 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<unsigned int>(unsigned int const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
0000000000016d90 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<short>(short const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
0000000000017400 W LercNS::ErrCode LercNS::Lerc::EncodeInternal<unsigned short>(unsigned short const*, int, int, int, int, int, int, unsigned char const*, double, unsigned int&, unsigned char*, unsigned int, unsigned int&)
000000000001a0f0 T LercNS::Lerc::Encode(void const*, int, LercNS::Lerc::DataType, int, int, int, int, int, unsigned char const*, double, unsigned char*, unsigned int, unsigned int&)

$ nm libLerc.so  | c++filt | grep "Lerc::.*Templ"
00000000000112a0 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<signed char>(signed char*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)
0000000000015310 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<double>(double*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)
0000000000014b00 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<float>(float*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)
0000000000011c40 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<unsigned char>(unsigned char*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)
0000000000013940 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<int>(int*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)
00000000000142f0 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<unsigned int>(unsigned int*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)
00000000000125e0 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<short>(short*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)
0000000000012f90 W LercNS::ErrCode LercNS::Lerc::DecodeTempl<unsigned short>(unsigned short*, unsigned char const*, unsigned int, int, int, int, int, int, unsigned char*)

This means that a user of the library cannot include these template functions in their own code as it leads to:
undefined symbol: _ZN6LercNS4Lerc11EncodeTemplIdEENS_7ErrCodeEPKT_iiiiiiPKhdPhjRj

and
$ c++filt _ZN6LercNS4Lerc11EncodeTemplIdEENS_7ErrCodeEPKT_iiiiiiPKhdPhjRj
LercNS::ErrCode LercNS::Lerc::EncodeTempl<double>(double const*, int, int, int, int, int, int, unsigned char const*, double, unsigned char*, unsigned int, unsigned int&)

@bretttully  The Lerc lib as built only exports the functions defined in the API. To call these medium level functions, the Lerc lib needs to be rebuilt. One way is to drop the C-API, add only the files you need to your own C++ project, then call these *Templ functions directly. Or, the shortest way to try this out:
_ in Lerc.h, add an export command, like this (I'm doing this on windows):
template<> (add "class T" inside the brackets)
__declspec(dllexport)
static ErrCode ComputeCompressedSizeTempl( ...
_ in main.cpp, add #include "../LercLib/Lerc.h"
_ in main.cpp, e.g., after the first lerc_Decode() call, add
unsigned int numberOfBytes = 0;
LercNS::Lerc::ComputeCompressedSizeTempl(zImg3, 5, 1, w, h, 1, 1, maskByteImg3, 0.1, numberOfBytes);
This builds and runs for me.

Yes, that makes sense -- I had just assumed that because these are public not private functions, they would be part of the API: https://github.com/Esri/lerc/blob/master/src/LercLib/Lerc.h#L154
Here is my example use case wrapping calls with numpy arrays with enforced data types.
#include "wrapLerc.hpp"

#include <pybind11/numpy.h>
#include <pybind11/stl_bind.h>

#include <LercLib/Defines.h>
#include <LercLib/Lerc.h>

namespace py = pybind11;

namespace {
static constexpr int LERC_VERSION = -1;  // use the latest

template <typename DataT> struct NumpyInfo {
    using NumpyT = typename py::array_t<DataT, py::array::c_style>;

    NumpyInfo() : info(), data(nullptr), nDim(1), nCols(0), nRows(0), nBands(0) {}

    NumpyInfo(NumpyT array)
        : info(array.request()), data(nullptr), nDim(1), nCols(0), nRows(0), nBands(0) {
        if (info.ptr == nullptr) {
            throw std::runtime_error("The array has no data!");
        }
        data = static_cast<DataT *>(info.ptr);
        if (info.ndim == 2) {
            nBands = 1;
            nRows = info.shape[0];
            nCols = info.shape[1];
        } else if (info.ndim == 3) {
            nBands = info.shape[0];
            nRows = info.shape[1];
            nCols = info.shape[2];
        } else {
            throw std::runtime_error("LERC compression requires a 2D or 3D array");
        }
        if (array.size() != nCols * nRows * nBands) {
            throw std::runtime_error("Failed to correctly extract array size");
        }
    }

    /// numpy buffer protocol
    py::buffer_info info;
    /// Raw image data, row by row, band by band. This is a raw pointer being kept alive by (at least) `this->info`
    DataT *data;
    /// number of values per pixel (for us, this is always 1)
    const int nDim;
    /// data shape
    int nCols, nRows, nBands;
};

/// Calculate the size of the required buffer size
template <typename DataT> struct LercBufferSize {
    using RasterInfo = NumpyInfo<DataT>;
    using RasterT = typename RasterInfo::NumpyT;

    static uint32_t calculate(const RasterT &raw, double maxZError) {
        RasterInfo raster(raw);
        return calculateImpl(raster, maxZError);
    }

    static uint32_t calculateImpl(const RasterInfo &raster, double maxZError) {
        // We don't care about the mask
        int nMasks = 0;
        LercNS::Byte *pValidBytes = nullptr;
        // size of outgoing Lerc blob
        uint32_t bs = 0;
        auto status = LercNS::Lerc::ComputeCompressedSizeTempl<DataT>(
            raster.data, LERC_VERSION, raster.nDim, raster.nCols, raster.nRows,
            raster.nBands, nMasks, pValidBytes, maxZError, bs
        );
        if (status != LercNS::ErrCode::Ok) {
            throw std::runtime_error("Failed to fetch the number of bytes required for the buffer");
        }
        return bs;
    }
};

template <typename DataT> struct LercCompressor {
    using RasterInfo = NumpyInfo<DataT>;
    using RasterT = typename RasterInfo::NumpyT;

    static void applyInPlace(RasterT &raw, double maxZError) {
        RasterInfo raster(raw);
        impl(raster, maxZError);
    }

    static RasterT apply(const RasterT &raw, double maxZError) {
        RasterT rawCopy(raw.request());
        RasterInfo raster(rawCopy);
        impl(raster, maxZError);
        return rawCopy;
    }

  private:
    static void impl(RasterInfo &raster, double maxZError) {
        // We don't care about the mask
        int nMasks = 0;
        LercNS::Byte *pValidBytes = nullptr;

        // --- get the buffer size
        uint32_t numBytesNeeded = LercBufferSize<DataT>::calculateImpl(raster, maxZError);

        // --- encode
        // buffer to write to, function will fail if buffer too small
        std::vector<LercNS::Byte> lercBlob(numBytesNeeded, 0);
        auto pLercBlob = lercBlob.data();

        uint32_t numBytesUsed = 0;  // num bytes written to buffer
        auto status = LercNS::Lerc::EncodeTempl<DataT>(raster.data, LERC_VERSION, raster.nDim, raster.nCols,
                                                       raster.nRows, raster.nBands, nMasks, pValidBytes, maxZError,
                                                       pLercBlob, numBytesNeeded, numBytesUsed);
        if (status != LercNS::ErrCode::Ok) {
            throw std::runtime_error("Failed to encode data");
        }

        // --- decode
        status = LercNS::Lerc::DecodeTempl<DataT>(raster.data, pLercBlob, numBytesUsed, raster.nDim, raster.nCols,
                                                  raster.nRows, raster.nBands, nMasks, pValidBytes);
        if (status != LercNS::ErrCode::Ok) {
            throw std::runtime_error("Failed to decode data");
        }
    }
};

template <typename DataT> void wrapLercImpl(py::module &m) {
    auto doc = py::doc("Note, it is assumed that if `raster.ndim == 3` it is arranged as "
                       "`raster.shape = (n_bands, n_rows, c_cols)`");

    using LBS = LercBufferSize<DataT>;
    m.def("buffer_size", &LBS::calculate, doc, py::arg("raster").noconvert(), py::arg("max_z_error"));

    using LC = LercCompressor<DataT>;
    m.def("apply_in_place", &LC::applyInPlace, doc, py::arg("raster").noconvert(), py::arg("max_z_error"));
    m.def("apply", &LC::apply, doc, py::arg("raster").noconvert(), py::arg("max_z_error"),
          py::return_value_policy::move);
}
}  // namespace

namespace cpputils {
namespace python {

void wrapLerc(py::module &m) {
    auto mLerc = m.def_submodule("lerc");
    wrapLercImpl<unsigned char>(mLerc);  // uint8_t
    wrapLercImpl<unsigned short>(mLerc);  // uint16_t
    wrapLercImpl<float>(mLerc);
    wrapLercImpl<double>(mLerc);
}
}  // namespace python
}  // namespace cpputils

@bretttully  So you suggest the use of PyBind11 for a more elegant and modern way to call from python into the Lerc lib. I'm not familiar with this, it looks interesting.
However, here are some arguments against it:
_ We want to have a general C API which can be used by any language, not just python.
_ So far Lerc has no external dependency. This would add one.
_ Lerc lib is small, has only a handful of API functions. The PyBind11 include files appear to be of roughly the same size as our Lerc lib (in lines of code).
_ The PyBind11 license file would need to be honored and made part of all software packages downstream. It would need to be added to the documentation of all software that uses Lerc.
To justify the use of PyBind11, it would have to make Lerc in python noticeably faster. So if you are game, feel free to continue along this line.

Sorry, I wasn't advocating you should move your code to pybind11, just showing an example of how I am using your C++ API in my own project (that does use pybind11 for a number of utilities I want to expose to python). That example code worked on 2.2 but now fails at runtime with 3.0 because the template functions are not exported to Lerc.so
That being said, conda/mamba/boa+pybind11 makes for a quite nice build and deploy of compiled python binaries so maybe I'll fork and play around. I have no idea how this would impact on the other languages, however.

repo: Esri/lerc


text:
Add support for editing tools/editing feature geometry

Describe requirements
We have heard requests for being able to edit geometry of features in Manager. This would also include adding the editing tools in the app too.
Now that we will have map based layout available I think adding support for editing geometry makes sense.
https://community.esri.com/t5/arcgis-instant-apps-questions/editing-geometry-in-new-manager-app/m-p/1392824#M2353
We used to have this functionality and  we disabled that in https://devtopia.esri.com/WebGIS/arcgis-portal-app-templates/issues/9018#event-23104403 in this issue and in this PR https://devtopia.esri.com/WebGIS/arcgis-portal-app-templates/pull/9034
Alternatives
No response
Other Relevant Info
No response
Impact
No response

Instant App team added a config option for allowing Editing geometry in the app.

@azizaparveen in terms of edit tools...do we have some design on where they would be displayed?

@azizaparveen in terms of edit tools...do we have some design on where they would be displayed?

@jmhauck no, not for this release, I discussed this with Sarah and we are ok with how it used to work before in R3, if I selected a record from the table or map it used to show the controls in the map for geometry editing, we think that should be enough for this release. Next release we are planning to add some selection tools and may be we can revisit this in R1.

This looks good in devext

source: gharchive/issue

authors: azizaparveen jmhauck

repo: Esri/solutions-components

license_type: permissive

license_source: github-api


text:
Reorganize project structure to be more monorepo like

Moves react app into its own directory like the api is.

https://github.com/Ethan-Arrowood/great-people/blob/453edb40843cddd3f8ce2f7add44051cfbc05118/.github/workflows/azure-fd-blob-fn-app.yml#L16
update the path to the react app in the repo

source: gharchive/pull-request

authors: Ethan-Arrowood peteroden

repo: Ethan-Arrowood/great-people

license: MIT


text:
Display error

This is the same game from different players, initially the wrong arrangement of chess pieces from one of the players, different display of moves.

Sorry I might be missing it, but where is it different? Or are you just noting that the background color of the tiles is different?

look from different players how the pieces stand, they are initially
located incorrectly for black
чт, 9 мар. 2023 г. в 19:54, jaredcosulich @.***>:

Sorry I might be missing it, but where is it different? Or are you just
noting that the background color of the tiles is different?
—
Reply to this email directly, view it on GitHub
https://github.com/EthosWallet/chess/issues/3#issuecomment-1462506259,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/A5AOVXWR7F4UAAHIF7MFPEDW3IKODANCNFSM6AAAAAAVVN2VSU
.
You are receiving this because you authored the thread.Message ID:
@.***>

the display is different for different players, the same pieces are on a white field for one and on a black one for another

if that helps then
0xa4f7492a88640fca3bd612170bfce40c8e1dfc73-white
0x7877011f2e900e55064602bff4a1be727d4a93f8 - black

Perfect, thank you for reporting this!

Oh no problem, glad to help, I love chess!

authors: jaredcosulich querisaiara

repo: EthosWallet/chess


text:
[Feature Request] Immediately show console instead of start window

It would be good to have an option (toggle) which can be activated in order to show the console window right after the start.
(btw. keep up the good work! This is the console I always wanted to have on windows where other console emulators look horrible)

Duplicate of #107

authors: Eugeny Simsch

repo: Eugeny/terminus

license: mit

license_source: bigquery


text:
Add back grid lines for DataArray plots

I think these were lost with the big refactor.

I probably forgot that when I branched the xarray case, LGTM

authors: JamesWrigley tmichela

repo: European-XFEL/DAMNIT


text:
动态更换颜色

既然都可以⌘+⌘-调字体大小 为什么不能调颜色呢
大概是给Menu里面添加一个列表显示所有颜色然后调整这样子

参考Stickies的⌘1-⌘6

repo: Ex-Studio/ExSticky


text:
add bsnes-hd

https://github.com/DerKoun/bsnes-hd

libretrocores done

standalone core will not start
sudo ./bsnes                     hiro: Gtk::cannot open display:
sudo xinit ./bsnes- black screen then crash
for now
libretrocore is the only thing working

repo: Exarkuniv/RetroPie-Extra


text:
Dynamic beam regression tests

This work contains several changes, most notably

Introduction of an abstract Forces class and a derived TimeVaryingForces to accommodate time-dependent loading as required by a regression test
Addition of LinearizationParameters class for dynamic beam elements
Couple of bug fixes in gen-alpha algorithm
Addition of several regression tests with validation results generated from BeamDyn

@ddement @deslaughter I made some minor modifications to the PR and it's ready for the final check.

repo: Exawind/openturbine


text:
Incompatible with Lunar Client

Description
Every time i want to play with this, there it just say "Your Client has crashed". After that i get again to the Client. I use all the mods that I need to use it. Please ! Bild_2024-09-23_184727996 me!
Minecraft version
1.21
Incompatible mod name
no one
Incompatible mod version
no one
Game Logs
1f622037-28b7-4b0f-9a8c-8fcd8d66a788-master

How do i get that?

Thank you for reporting. Unfortunately, the code does not help. I need the full log file in order to analyze the issue.

How do i get that?

Sorry for the late answer but here is the link https://mclo.gs/TZi4PNz . Hope it helps you.

Thank you for the logs. Lunar client has a built-in mod (lunar) that causes the crash. Unfortunately, due to the way the mod is made, it is impossible to resolve. Maybe you can report this to the lunar client devs. The only thing I could do is making a modification to the Minecraft source code by Shoulder Surfing optional. BUT, this could cause the crosshair to not be visible in Shoulder Surfing perspective, which I'm not really a fan of.

I understand, thank you that you took time for me.

authors: Exopandora LarsMagVegas

repo: Exopandora/ShoulderSurfing


text:
Adding feature for superadmin to be able to edit player inventory

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
Describe the solution you'd like
A clear and concise description of what you want to happen.
In the database there is a possibility to remove items from the players inventory including the house, vehicles as well as the character inventories. There could be an integration in the web panel to be able to remove or delete items from the inventory via the database without having to use the database directly thus this feature can be limited to Super admins. This feature can be added in the character section perhaps or in the players profile. This can make it easier during inventory wipes for random looting cases or intentionally wiping someones inventory for gain rather than calling them to support and wiping it this feature would make it easier.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Having the player join support and wiping inventory but if you have that happening everytime and you got a bigger player base this would make it a long process because people being unavailable this would just let a super admin to wipe the inventory and would be more efficient.
Additional context
Add any other context or screenshots about the feature request here.

Would have to get a sign off from @InZidiuZ for that

Super admins having the ability to do something like this wouldn't be a concern.

Implemented in the latest update

authors: GuArDiAn-ops InZidiuZ milan60


text:
[PAYMENT - 01-25] Double Tapping On Invite Button Navigates Back To The Selected WorkSpace Menu

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:

Go to Settings
Select Workspaces
Click On Invite User
Select User To Invite
Double Tap On Invite Button

Expected Result:
It should navigate navigate to Manage Members Screen
Actual Result:
It navigates user back to Workspaces
Workaround:
unknown
Platforms:

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

[ ] Android / native
[ ] iOS / native

Version Number: 1.2.53-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/212368205-f3893b5b-98f9-4803-825a-56ce9c1e5e0f.mp4
https://user-images.githubusercontent.com/43996225/212368343-b774dd72-c1ff-4dc7-85d1-1c0338ee1219.mp4
Expensify/Expensify Issue URL:
Issue reported by: @syedsaroshfarrukhdot
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673578313575849
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01eb6bd6d09d862e08
Upwork Job ID: 1613998991762903040
Last Price Increase: 2023-01-13

PR up

Invited @eVoloshchak via Upworks

@jliexpensify as you pointed out via 1:1, @sobitneupane was the reviewer for this PR, so I'm reassigning this issue. Sorry about the confusion everyone!

Invited @sobitneupane , sorry Eugene!

Looks like @sobitneupane qualifies for the bonus

@jliexpensify It was an internal PR. Are internal PR reviews eligible for bonus?

Good question - let me check that, given it's an internal one. Thanks Sobit!

Ok, my mistake @sobitneupane - just a flat rate. Sorry!

Are we still waiting on payments?

I'm OOO tomorrow but will pay once I'm online again

Just hired Sobit, once he accepts I'll sort out $

@jliexpensify Pinging for reporting bonus.

@syedsaroshfarrukhdot - this is you, right?

Yes

Thanks for invitation just accepted the offer.

Just hired Sobit, once he accepts I'll sort out $

Thanks @jliexpensify. Accepted the offer.

Paid Sobit, just waiting on Syed to accept. Cheers!

Paid Sobit, just waiting on Syed to accept. Cheers!

Accepted.

Paid, and job closed. Thanks Sobit and Syed!

authors: jliexpensify kavimuru luacmartins sobitneupane syedsaroshfarrukhdot

repo: Expensify/App


text:
[$250] Migrate SignInPage.js to function component

Class Component Migration
Filenames

[ ] src/pages/signin/SignInPage.js

type: React.Component
has state values: false
has refs: false
has context: false
lifecycle methods: componentDidMount

Task

We currently have some class components in our codebase that we would like to refactor to a function component.
Here's a link with some general advice on how to refactor a class component to a function component: https://react.dev/reference/react/Component#alternatives
If you need additional guidance, please ask in #expensify-open-source
Test for any regressions and verify that there are no breaking changes

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0184fa212744ee42dc
Upwork Job ID: 1668429651374313472
Last Price Increase: 2023-06-13

⚠ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

Heads up! The pricing for this issue has been adjusted based on the scope of the work and the fact that no complex bug investigations or proposal are required.

@puneetlath seeing as you've completed your first refactor issue I'm going to take this one off HOLD now.

@roryabraham actually asked to take this one over, so assigning to him.

Was working on a related bug but then didn't end up needing the changes in this component. I'll still finish this out, but it's not urgent.

Created a PR but brought a few questions to slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1684879407105369

Updated my PR to resolve conflicts, now just needs screenshots

PR ready for review

This was just deployed to staging, marking this as Internal to get a BZ assignee and upwork job created for the C+ payment

Assigning @sobitneupane since they reviewed the PR already

Also hmmm why did that not give me a BZ assignee? Maybe if I add the New Feature label?

Ok, hey @kevinksullivan! The only payment due here is $250 to @sobitneupane 1 week after https://github.com/Expensify/App/pull/19498 hits production.

This was reverted

Hey team! It looks like we might have a regression on this PR - can you check this and see if it should be treated separately?
https://github.com/Expensify/App/issues/20710

@Christinadobrzyn The PR for this issue was reverted.

Only a 1-line change was needed, but I need to do screenshots again on the redo PR. Will shoot to do that early next week

Tracking this for https://github.com/Expensify/App/issues/20710 - what is the update here? What PR are we following?

authors: Christinadobrzyn MelvinBot marcaaron puneetlath roryabraham sobitneupane


text:
[HOLD for checklist] [$1000] Attachment downloaded when user clicked outside the attachment button

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:

Open staging.new.expensify.com
Send an attachment to a chat
Click the empty area outside the attachment button

Expected Result:
Attachment file shouldn't be downloaded since the user clicked outside the button
Actual Result:
Attachment file downloaded and could potentially caused confusion
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.19-2
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: Any additional supporting documentation
https://github.com/Expensify/App/assets/43996225/868e6315-69af-4fc0-aa71-30447c0467a0
https://github.com/Expensify/App/assets/43996225/251e247a-d594-4ad1-862c-579038a0bacb
Expensify/Expensify Issue URL:
Issue reported by: @kerupuksambel
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684984471329289
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e0f07bfc53f77442
Upwork Job ID: 1663611880864784384
Last Price Increase: 2023-05-30

This is kind of a duplicate of #19017. I'd be happy to fix all occurrences of this. My proposal - https://github.com/Expensify/App/issues/19017#issuecomment-1549907508.

@hoangzinh I believe in both cases the entire light green portion ONLY is should be clickable. The same issue also exists for displayable attachments such as images and PDFs.

I reproduced this - and I tend to agree it looks like the same root cause as https://github.com/Expensify/App/issues/19017

I'm asking the team to confirm

Proposal
Please re-state the problem that we are trying to solve in this issue.
We can click outside of attachment box to download it.
What is the root cause of that problem?
This is a regression of https://github.com/Expensify/App/pull/17496 which we previously set align-items: start to the HTML renderer default view. Imagine that the attachment box (and its Pressable) is wrapped by a View that has a style of align-items: start. Now, it's removed and moved to the Pressable. So, the Pressable have the full width, while the attachment box still stay the same because we have the align-items: start on the Pressable.
https://github.com/Expensify/App/blob/619a5298771ba5e42ae6f1596a37e32b9cd16fc0/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js#L81-L82
https://github.com/Expensify/App/blob/619a5298771ba5e42ae6f1596a37e32b9cd16fc0/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.js#L48-L49
This also happens with ImageRenderer.
What changes do you think we should make in order to solve the problem?

Wrap the Pressable with View and move the styles.alignItemStart there.

Okay, keeping this separate! Applying External accordingly

@thesahindia thoughts on the proposal here?

Ah yes, sorry @hoangzinh

Thanks for taking care of that @grgia! We're on our way. 👍

Having some issue on my local. I will probably open the PR tomorrow (at my time).

PR is up, review underway

PR merged, awaiting deploy to staging -> prod

cc @grgia

This might be a revert candidate

@greg-schroeder looking into this more, but at a glance, I agree

@youssef-lr I believe that's it. I think that's probably the best bet cc @thesahindia

I think we should exclude PR https://github.com/Expensify/App/pull/19545 as it seems unrelated to this issue and is fixing other issues, and remove the troublesome line causing https://github.com/Expensify/App/issues/20669. For the other two we can revert. What do you think @grgia?

@thesahindia that one is the culprit for this https://github.com/Expensify/App/issues/20669, but only a troublesome line there is causing the issue. I don't think we need to revert the full PR as it's fixing other stuff.

If I understand correctly, https://github.com/Expensify/App/issues/20669 is caused by https://github.com/Expensify/App/pull/19545 and https://github.com/Expensify/App/issues/20566 is caused by our PR https://github.com/Expensify/App/pull/20440 https://github.com/Expensify/App/pull/20114
In our case, we still need the changes from our PR. I have found a fix for this regression https://github.com/Expensify/App/issues/20566. Can I open the PR now?

Open it anyway as it's EOD for me.

Discussing with the team whether we go for a revert or your fix.

What's going on with this issue? It's kinda difficult to follow what happened re: reverting all or part of what caused the deploy blocker :/
cc @grgia @youssef-lr @thesahindia

@youssef-lr bump!

Thanks for that @bernhardoj

This issue is very difficult to follow given all the regressions, but here's the timeline as far as I can tell:
June 2nd: PR Created
June 7th: PR Merged
June 8th: Regression found
June 13th: New PR created to fix regression
June 27th: PR Merged
June 29th: Regression found
June 30th: New PR created to fix regression
July 6th: PR Merged
I'm removing the payment hold waiting for this newest PR to be deployed, as that'll create a new payment hold.
I have no idea how we're going to pay this one out though, tbh. cc @grgia @thesahindia

Okay so we're finally ready for this one I believe!

@grgia @youssef-lr Per my comment here, a LOT happened with this issue...
Can anyone weigh in on the best way to pay this out? I'm honestly not sure because so many things went wrong here. There aren't really clear guidelines when so many things went awry and so many people were involved 😅

Hmm, I need to create a new upwork job I guess. Doing.

Sent offers to @bernhardoj @kerupuksambel. Given there were two regressions the 50% penalty was applied twice
@thesahindia you'll handle payment via newdot!

Payments are complete, waiting for checklist @thesahindia

Bump on the above @thesahindia :)

authors: bernhardoj greg-schroeder grgia kavimuru therealsujitk youssef-lr


text:
[$1000] Contact method - Error message about existing account disappears after refreshing

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:

Go to staging.new.expensify.com
Go to Settings > Profile > Contact methods > New contact method
Add an existing account
Save it
Refresh the page

Expected Result:
The error message that the existing account cannot be used will not disappear after refreshing the page
Actual Result:
The error message that the existing account cannot be used disappears after refreshing the page
Workaround:
Unknown
Platforms:

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

[ ] Android / native
[x] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.39.5
Reproducible in staging?: Yes
Reproducible in production?: Yes
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: Any additional supporting documentation
https://github.com/Expensify/App/assets/78819774/4672e7a1-7445-4dd7-b919-6a6ca9a32f75
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~018dd619d053797a3c
Upwork Job ID: 1679219149348700160
Last Price Increase: 2023-07-12

This should be fixed in the backend. ReconnectApp API returns loginList and it doesn't include failed contacts.

This feels like a bug to me. If you navigate away from the this error message, we continue to show it if you then go back to the page. It feels odd that we'd then remove the error entirely (without the user acknowledging it) if you refresh the page. The behavior should be consistent.
https://github.com/Expensify/App/assets/54866469/682fe74e-e0e4-4260-99b0-49ff924aac80

@Santhosh-Sellavel I think this one might be fixed by https://github.com/Expensify/App/issues/17827 , curious if you agree?

Yes seems like a duplicate

Cool, thanks! Going to close, as this is a dupe of the above issue.


text:
[$500] Request money - Description briefly displayed in preview after adding a receipt from a manual request

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:

Open a chat.
Navigate to the "" option.
Enter the desired amount and click "Next"
Click on the three dots menu and select "Add receipt" and select a file
Enter a description and save it.
Now send the request.

Expected Result:
The description should either be removed from the preview or not displayed at all
Actual Result:
The description is briefly displayed in the preview when adding a receipt from a manual request
Workaround:
Unknown
Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: v1.3.74-2
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: Any additional supporting documentation
https://github.com/Expensify/App/assets/115492554/3f9a2d10-0ba2-4587-a2e1-decfac909665
https://github.com/Expensify/App/assets/115492554/aeb12f0c-c925-4cd5-a6d6-084e35097d77
Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695666475040319
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a6766bd5a43af155
Upwork Job ID: 1707332794345508864
Last Price Increase: 2023-09-28

Proposal
Please re-state the problem that we are trying to solve in this issue
The description is briefly displayed in the preview when adding a receipt from a manual request.
What is the root cause of that problem?
The problem comes from the ReportPreview.js component:
{!isScanning && (numberOfRequests > 1 || hasReceipts) && (
    <View style={styles.flexRow}>
        <View style={[styles.flex1, styles.flexRow, styles.alignItemsCenter]}>
            <Text style={[styles.textLabelSupporting, styles.mb1, styles.lh20]}>{previewSubtitle || moneyRequestComment}</Text>
        </View>
    </View>
)}

This code block is supposed to render the number of requests if multiple requests exists. The problem is the OR operator between these two variables previewSubtitle || moneyRequestComment, initially previewSubtitle is empty hence why the description shows up because moneyRequestComment is being shown.
This changes as soon as previewSubtitle is not empty anymore and is replaced by the number of requests label.
What changes do you think we should make in order to solve the problem?
I would create another component below the existing one where we will always render the request description like so:
{moneyRequestComment && (
    <View style={styles.flexRow}>
        <View style={[styles.flex1, styles.flexRow, styles.alignItemsCenter]}>
            <Text style={[styles.textLabelSupporting, styles.mb1, styles.lh20]}>
                {moneyRequestComment}
            </Text>
        </View>
    </View>
)}

What alternative solutions did you explore? (Optional)
N/A
Videos
https://github.com/Expensify/App/assets/56457735/05487e8d-222d-40f9-8b36-88d22420faf2

Taking this over based on @ntdiary's request

sorry, I missed this due to travel at the weekend. BZ team review complete.

@situchan - what do you think of the current proposals?

@s-alves10's proposal looks good to me.
I think we should go with Solution 1 which fixes optimistic data as sending Merchant value in backend seems intentional.
Assigned engineer will confirm this in backend logic.
🎀 👀 🎀 C+ reviewed

@peterdbarkerUK can you please assign me to this issue?

hmm dang something might have gotten borked with the c+ switch-a-roo but @s-alves10 👍 go ahead with your proposal 1 to just fix this on the front end.

@situchan
PR is ready for review https://github.com/Expensify/App/pull/29118

Thanks Bondy!

ooops my bad, not deployed to staging yet.

Summarizing payouts for this issue:
Issue reporter: @ayazhussain79 $50 (offer sent)
Contributor: @s-alves10 $750 (inc urgency bonus)
Contributor+: @situchan  $750 (inc urgency bonus) - please complete the BZ checklist and notify me here so that I can complete the payout.
Upwork Job

@situchan could you complete the checklist?

(Just a note for myself that ohter contribs have been paid, and contract closed)

[x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/25924
[x] 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: https://github.com/Expensify/App/pull/25924/files#r1377800775
[x] 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: N/A
[x] Determine if we should create a regression test for this bug.
[x] 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.

Regression Test Proposal

Go to FAB > Request money > select Manual
Input any amount and click Next
Click on the three dots menu and select "Add receipt" and select a file
Enter a description and save
Tap "Request"
Verify that description is not shown and "Request" is shown in preview

@peterdbarkerUK sorry for late. Can you also help with payment on #27710 please?

@peterdbarkerUK can you please check you have ended contract on upwork without paying

Ah shoot, my mistake @ayazhussain79 - I'd had to add you to the job and didn't correctly pay it out when closing. I've sent you a new contract offer, bump me here once you've accepted it!

@peterdbarkerUK Np, Offer accepted and thank you

Paid, thanks Ayaz!

@situchan I've updated the original issue, thanks for raising it and I'm sorry for the error.

No problem. Thanks
Also payment for this one (https://github.com/Expensify/App/issues/28380) is pending

Haha one at a time! I'm working through these but being interrupted by an ongoing interview ;-)

Paid! Leaving open so I can sort out the regression testing tomorrow.


text:
Chat - Participants screen briefly shown and distance page opens few times after opening link

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.3.96.0
Reproducible in staging?: y
Reproducible in production?: n
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Issue found when executing PR https://github.com/Expensify/App/pull/28308
Action Performed:

Open the app
Tap FAB > Request money > Distance
Navigate to https://staging.new.expensify.com/request/new/participants

Expected Result:
User is navigated to the distance request page
Actual Result:
Participants screen briefly shown and distance page opens few times after opening link
Workaround:
Unknown
Platforms:

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

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

Screenshots/Videos

https://github.com/Expensify/App/assets/93399543/aee67024-7db1-43d4-b700-4aa0943f9e49

View all open jobs on GitHub

This is the only PR that obviously looks related to deep linking:
https://github.com/Expensify/App/pull/29636/files

It doesn't really seem like it would be touching this distance request code flow though.

I'm gonna demote this one for the following reasons:

The problem is only affecting native apps
The bug is only reproducible if you do something "weird"
Deep linking to the participants page is kind of "holding it wrong". I can't think of a clear reason why someone would actually do that.

Going to close and we can reopen if a real user hits this

authors: kbecciv marcaaron muttmuure


text:
[$500] Web – Chat – Blue frame appears around message Reply when open context menu & use keyboard arrow

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.6-1
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:

Go to https://staging.new.expensify.com/
Log in with any account.
Go to any chat that has a message with Reply
Right-click the message
Verify that context menu opens
Repeat pressing up/down arrows from keyboard

Expected Result:
Blue frame does not appear around message Reply when open context menu and use up/down arrows from keyboard
Actual Result:
Blue frame appears around message Reply when open context menu and use up/down arrows from keyboard
Workaround:
Unknown
Platforms:

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

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

Screenshots/Videos
Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/189c89f0-0205-4c26-94f1-b263e084cf8a
View all open jobs on GitHub

@getusha Please assign it to me

I  think this one already sorted.

It is working good.
https://github.com/Expensify/App/assets/75600326/0dc922c3-9bcc-4701-8e5d-c3cf040a1909

Contributor details
Your Expensify account email: <sauravmdh25@gmail.com(mailto:sauravmdh25@gmail.com)>
Upwork Profile Link: https://www.upwork.com/freelancers/~015a4060b1006352d5

Contributor details
Your Expensify account email: sauravmdh25@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~015a4060b1006352d5

Proposal
Please re-state the problem that we are trying to solve in this issue.
Web – Chat – Blue frame appears around message Reply when open context menu & use keyboard arrow
What is the root cause of that problem?
There's no 'box-shadow: none'style for this component:
https://github.com/Expensify/App/blob/2e8de3ffd53f40139a927bc60461feadefe88d78/src/pages/home/report/ReportActionItemThread.js#L46
What changes do you think we should make in order to solve the problem?
We can simply add the style to this comeponent by adding style={[ styles.noSelect ]}
https://github.com/Expensify/App/blob/2e8de3ffd53f40139a927bc60461feadefe88d78/src/pages/home/report/ReportActionItemThread.js#L46
Result
https://github.com/Expensify/App/assets/91727683/f25d0541-edfe-4998-8564-d6f28cdbd185
What alternative solutions did you explore? (Optional)
N/A

We aren't prioritizing keyboard accessibility issues at the moment. Fine to close

unassigned my self as i am not able to get windows device to test this, @situchan offered to take this.

I think the issue is that popover is not focused on right click.
i.e. when press Tab key, first focus on composer + rather than reaction icon in popover
https://github.com/Expensify/App/assets/108292595/b4986a0a-0d05-42c3-8602-81a11ee84762

Before going ahead, I'd like to confirm with engineer if this is worth fixing as this is accessibility issue
🎀 👀 🎀

I think accessibility issues are important to fix, so let's move forward with a proposal here, @situchan

Still waiting on proposal review

Ah sorry was out of my radar as not assigned. Reviewing shortly

@tienifr can you share other instances where your solution is applied to fix similar issues?

Why can't we fix https://github.com/Expensify/App/issues/32279#issuecomment-1853583261 which is the actual root cause?

I agree with @situchan that we should fix the root cause, which is the focusing the popover first.

Proposal
Please re-state the problem that we are trying to solve in this issue.
Chat – Blue focus (from browser) accessibility frame appears around message Reply when open context menu & use keyboard arrow
What is the root cause of that problem?
When a user interacts with the context menu in the application using keyboard navigation, an unintended blue focus frame (browser's default focus style) appears around the message reply button. This occurs specifically when the user opens the context menu and navigates using the keyboard arrows.
This issue arises due to the way browsers handle focus on elements. When the context menu is activated (via a right-click on a message reply), the focus remains on the originally clicked element in the DOM, which is the reply thread in our case. As the user navigates through the context menu using the keyboard, the focus style applied by the browser remains visibly attached to the reply thread (button), leading to a confusing user experience.
What changes do you think we should make in order to solve the problem?
Adjust Focus Behavior: Modify the focus behavior when the context menu is triggered. Currently, when a user right-clicks on the message reply, the onSecondaryInteraction event is triggered, which calls the showPopover function to display the context menu.
https://github.com/Expensify/App/blob/7ae1ec7e74b908e6340bccaf7e42b0345d766711/src/pages/home/report/ReportActionItem.js#L255-L261
Code Modification: In the showPopover function, introduce a new snippet to manage the focus state (we already use this in other components):
if (DomUtils.getActiveElement()) {
  DomUtils.getActiveElement().blur();
}

This code ensures that the focus is removed from the message reply DOM element when the context menu is activated, thereby preventing the display of the browser's default focus style on the reply button.
What alternative solutions did you explore? (Optional)
Third-Party Library Integration: One alternative could be to integrate a third-party library specifically designed to manage focus states and facilitate proper tab navigation. However, this approach may be overly complex for addressing a relatively minor issue. It would also necessitate extensive modifications to other parts of the application's tab navigation system.

reviewing new proposals

@tienifr please do regression test with your new solution as it affects all popovers throughout the app.
focus-trap-react module is being added in #32800 which is 3rd attempt (because of 2 regressions) of fixing #15631.

Just FYI, we already have a proposed solution here of wrapping the popover with focus trap which is currently on hold for https://github.com/Expensify/App/issues/15631

Same

Still holding for focus trap v3

Same

still holding

Based on https://github.com/Expensify/App/pull/32800#issuecomment-1924310126, looks like holding PR is going to be closed.

@roryabraham looks like we can close out the PR, does that mean the issue can also be closed out?

The linked issue fixed during navigation refactor but this bug still exists though

@situchan sounds good, let's open this back up for proposals

Proposal
Please re-state the problem that we are trying to solve in this issue.
Blue frame on wrong focused element due to right click.
What is the root cause of that problem?
This issue is not happening only on the element the QA mentioned on the issue. This happens with alot of element inside the report screen. Below some:

icon on comment:

profile picture

.. and more

I  tested  <FocusTrap> and it's not solving the issue.
I also tested withoutFocusOnSecondaryInteraction and it's removing the focus from the selected element but when you click on TAB from keyboard, the focus goes to other wrong element on the report screen.
The cause of the issue is that when the Report Action Context Menu is opened, the focus is not inside the component, it still outside of it.
What changes do you think we should make in order to solve the problem?
We need to set the focus inside the PopoverReportActionContextMenu so when we use keyboard keys, we navigate to the elments inside the component.
below a part of the main code change.
We need to set focus on the 1st element from the component here
https://github.com/Expensify/App/blob/9a598e0437cecb9bc23a57d7f15b59d496b69631/src/components/Reactions/QuickEmojiReactions/BaseQuickEmojiReactions.tsx#L34-L38
- {CONST.QUICK_REACTIONS.map((emoji: Emoji) => (
+ {CONST.QUICK_REACTIONS.map((emoji: Emoji, index: number) => (
  <Tooltip
   text={`:${EmojiUtils.getLocalizedEmojiName(emoji.name, preferredLocale)}:`}
   key={emoji.name}
  >
-  <View>
+  <View  ref={index === 0 ? firstEmojiRef : null}>
    <EmojiReactionBubble
      emojiCodes={[EmojiUtils.getPreferredEmojiCode(emoji, preferredSkinTone)]}
      isContextMenu
      onPress={Session.checkIfActionIsAllowed(() =>  onEmojiSelected(emoji, emojiReactions))}
    />
  </View>
  </Tooltip>
))}

Once we get the ref of the 1st element, we need to set focus to it:
useEffect(() => {
  // Set focus to the first emoji element
  if (!firstEmojiRef.current) {
   return;
  }
  const  buttonElement  =  firstEmojiRef.current.querySelector('button');
  if (buttonElement) {
   buttonElement.focus();
  }
}, []);

With the update below, once the Popover is opened,  we now have the focus set to the element we want.
POC (I let the focus with the blue border for the demo video to show how it's working):
https://github.com/Expensify/App/assets/12425932/6d56e724-597e-470c-a2d2-cc04c469099b
The DEMO was a quick code edit, deep details can be handled throw the PR.

New proposal here

@dragnoir thanks for the proposal but it looks like workaround. I don't like the idea of getting ref of first element.
Looking for more general solution.

Hey @situchan. What about my solution. You didn't give a review for it so that I update it. Could you check it and give a review pls?

@MahmudjonToraqulov please try to find general solution. Not only happening on thread item
https://github.com/Expensify/App/assets/108292595/a202b5f7-2c09-427a-b63f-3fdc65193f9f

@situchan pls check my new alternative solutions

So @mountiny this is keyboard/tab related. Are we going to hold these and move them to a more broader project?

I think we can continue.
I asked here and here's the answer

@situchan this is a video that shows the part I mentioned in my alternative solution (LHN) and we should follow
https://github.com/Expensify/App/assets/12425932/23beb8c7-34ae-4f05-98a1-2bd1c1d5def1

I think we can continue.
I asked https://github.com/Expensify/App/issues/32279#issuecomment-1853585953 and here's the https://github.com/Expensify/App/issues/32279#issuecomment-1858525238

Okay, but thinking about the wider context. We haven't put an emphasis on accessibility and particularly keyboard/tab navigation throughout the app. There's a ton of places where we have "bugs" in this regard. Equally, we don't have anything in the checklist or guidelines for developing to ensure we're always accounting for this navigation -- so we'll forever have "new bugs" as a result of not officially supporting it and putting a framework/guidelines around it.
Fixing these issues "one off" has diminishing returns, so a better approach to truly support this consistently throughout the app would to tackle it holistically and methodically. @mountiny @puneetlath @iwiznia and I discussed that a bit internally, and I've moved the output of that to here to see if we can get the ball rolling to put this to bed once and for all.

@trjExpensify thanks for the summary. So you suggest to hold this issue for https://github.com/Expensify/App/issues/36476, right?

I agree with the hold

Still on HOLD

Same

Held on #36476 still.

Samesies.

Held on https://github.com/Expensify/App/issues/36476 not overdue

Same!

Still held on https://github.com/Expensify/App/issues/36476

Still held, Melv. @jnowakow is picking up from Adam on https://github.com/Expensify/App/issues/36476#issuecomment-2095496118

Same, Melv.

Same ^

No change.

https://github.com/Expensify/App/pull/39520 which fixes #36476 was deployed to prod on Jun 14.

Bug is still reproducible

Proposal
Please re-state the problem that we are trying to solve in this issue.
Blue outline appears around the reply button when pressing arrow up key while context menu is open.
What is the root cause of that problem?
When we right click the reply/replies button, the button is focused. If we press any key, the blue outline will appear around the focused element. It's supposed to be fixed by a focus trap PR so the context menu will be focused when opened, but then we decided to not focus the context menu/any modal initially in this PR.
However, if we use the arrow down key, then the blue outline around the reply/replies doesn't appear. Why? It's because pressing the arrow down key will highlight and focus (thanks to useSyncFocus) the first context menu item, but if we use the arrow up key, none of the context menu items are highlighted and focused.
Previously, if we press the arrow up key, the last item will be highlighted and focused, but in this commit, we disable the cyclic traversal of the context menu arrow key.
https://github.com/Expensify/App/blob/b33542383ac2cb8ea1e922aa12dd2265efccd0ab/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L167
What changes do you think we should make in order to solve the problem?

Enable the cyclic traversal back by removing this code.
https://github.com/Expensify/App/blob/b33542383ac2cb8ea1e922aa12dd2265efccd0ab/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L167
https://github.com/Expensify/App/assets/50919443/eee2f234-62e3-4dcf-a9fe-fa34df35a4e7

@bernhardoj thanks for following this issue.

Previously, if we press the arrow up key, the last item will be highlighted and focused, but in this commit, we disable the cyclic traversal of the context menu arrow key.

If it was intentionally disabled, then won't enabling back cause regression?

Bump, @jnowakow. Looking for input on this.

Still waiting on a response from @jnowakow

Hello, sorry that you had to wait so long for me.
The cyclic travers was disabled because it would break the tab navigation. When last element was focus clicking tab would move focus to first emoji but also highlight the first element of list which would be confusing. Without cyclic travel there's no such problem

I see. Thanks for the explanation, but I tested it out and it works fine. I think the issue doesn't persist anymore.
(cyclic traversal enabled)
https://github.com/Expensify/App/assets/50919443/e8517dac-c059-40fa-a2d1-c46f05b228e4

Okay, so what are the next steps here?

I think we need @situchan approval

@bernhardoj's proposal looks good to me.
🎀👀🎀 C+ reviewed

PR is ready
cc: @situchan

I'll request in ND once payment is due.

The payment should be due tomorrow (07-17).

Requested in ND.

Yup, awaiting the checklist!

Not able to find offending PR. I think this bug existed from the beginning.
As this is accessibility issue, I don't think it's worth adding regression test.

Alright, payment summary as follows:

$500 to @bernhardoj for the fix (go ahead and request)
$500 to @situchan for the C+ (paid via upwork!)

Internal note: The Upwork contract has a start date of 1st July, and Bernhard was eligible from 27th June according to this SO).
Closing!

$500 approved for @bernhardoj

authors: Chetax JmillsExpensify MahmudjonToraqulov SSRV25 bernhardoj dragnoir getusha jasperhuangg jnowakow lanitochka17 mountiny situchan trjExpensify


text:
Distance - Distance rate changes to personal expense distance rate after editing distance

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.52-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team
Action Performed:

Go to staging.new.expensify.com.
Create a new workspace.
Go to workspace chat.
Create a distance request.
Go to distance request details page.
Note that distance rate.
Click Distance.
Edit the distance and save it.

Expected Result:
The distance rate for the new distance will follow the workspace distance rate.
Actual Result:
The distance rate for the new distance follows the personal expense distance rate after changing distance.
If the distance request is edited on Collect workspace as employee, there will be a red dot next to transaction thread in LHN.
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/247b5019-7f96-473d-91ee-b0b218a4eb45

View all open jobs on GitHub

@puneetlath 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.

We think that this bug might be related to #wave-collect - Release 1
CC @trjExpensify

Looks like this is a back-end issue. @neil-marcellini I see you recently have been working on P2P distance. Any ideas about the cause?

If it's P2P Distance related that project is moving to #vip-vsb. 👍

It actually affects workspace requests :(

Yeah, being discussed here: https://expensify.slack.com/archives/C07J32337/p1710440479773439

A fix has been sent to Web staging. Can you please re-test?

Note that this bug also only occurs if your personal workspace and workspace requested from have different reporting currencies. The easiest way to reproduce is to change the currency of the workspace to something other than USD.

Just tested and it looks like it's working properly now. Closing this out!

authors: izarutskaya neil-marcellini nkuoch puneetlath trjExpensify


text:
[HOLD for payment 2024-12-07] [$250] Categories - Error in Empty Category Not Dismissed

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: v9.0.64-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+tw4355353333@applause.expensifail.com
Issue reported by: Applause Internal Team
Action Performed:

Go to Settings > Workspace > Categories.
Delete all categories and import a CSV file.
Select a field to map from the spreadsheet (e.g., Amount), enable it, and save
Observe the error message. Click the "Close" button to dismiss the error.

Expected Result:
The error message should be dismissed upon clicking the "Close" button.
Actual Result:
The error message remains displayed even after clicking the "Close" button.
Workaround:
Unknown
Platforms:

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

Screenshots/Videos
https://github.com/user-attachments/assets/50ab8270-c0c4-4744-8cd2-e0694c6801b2
Bug6669395_1732017654330!Bulk_Export_id_DEFAULT_CSV__1_.csv

View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021859068445047163468
Upwork Job ID: 1859068445047163468
Last Price Increase: 2024-11-20
Automatic offers:

brunovjk | Reviewer | 105006793
abzokhattab | Contributor | 105006794

Issue OwnerCurrent Issue Owner: @kadiealexander

Proposal
Please re-state the problem that we are trying to solve in this issue.
Error in Empty Category Not Dismissed
What is the root cause of that problem?
We are unable to close this error because we can't find the category. As a result, we return early and cannot set the error to null to hide the error message.
https://github.com/Expensify/App/blob/faa48b605d708d7e8814e54fbf2b88a883c9c0c0/src/libs/actions/Policy/Category.ts#L845-L857
https://github.com/Expensify/App/blob/be6acac6f051b290d96e14353704065e4928b53a/src/components/OfflineWithFeedback.tsx#L147-L154
We can't find the category because we don't have a category name. The reason we don't have a category name is that the imported CSV does not include a name field.
What changes do you think we should make in order to solve the problem?
To fix this issue, we shouldn't accept a CSV containing invalid data. We should check whether the CSV includes a name field or not. If the CSV does not include a name field, we should return early and display an error message. Something like this:
// src/components/ImportSpreadsheet.tsx#L96
+    const rowTitle = data.at(0) as string[];

+    if (!rowTitle.includes(CONST.CSV_IMPORT_COLUMNS.NAME)) {
+        setUploadFileError(true, 'spreadsheet.importFailedTitle', 'spreadsheet.invalidFileMessage');
+        setIsReadingFIle(false);
+        if (fileURI && !file.uri) {
+            URL.revokeObjectURL(fileURI);
+        }
+        return;
+    }

Test branch

POC
https://github.com/user-attachments/assets/4f62c871-dc02-40ac-96a5-2c60d2b7c3b7

Reviewing proposals soon.

Thank you all for the proposals.
@Nodebrute, I think your proposal alone is not enough as it doesn’t address the root cause of allowing empty categories during CSV import. @Abzokhattab, @huult, and @nkdengineer, correct me if I’m wrong, but your proposals seem similar in that they focus on updating the CSV import validation.
With that, I think we should go with @Abzokhattab's proposal, as it is the first robust solution addressing the root cause effectively while maintaining user clarity.
🎀👀🎀 C+ reviewed

@brunovjk Yes, thank you for your feedback. For my proposal, I suggest notifying the user about invalid steps during the CSV import process to enhance the user experience. Regarding the proposal you voted for, validation occurs after the import. However, since editing the data fields is not possible at that step and validation is always shown but cannot proceed, it becomes redundant.

Thank you all for the proposals.
@Nodebrute, I think your proposal alone is not enough as it doesn’t address the root cause of allowing empty categories during CSV import. @abzokhattab, @huult, and @nkdengineer, correct me if I’m wrong, but your proposals seem similar in that they focus on updating the CSV import validation.
With that, I think we should go with @abzokhattab's proposal, as it is the first robust solution addressing the root cause effectively while maintaining user clarity.
🎀👀🎀 C+ reviewed

I agree, it's a good proposal and resulting UX. Hiring.

Hey @abzokhattab :D just checking, would you happen to have an ETA for this PR? Thanks.

Update: PR still under review.

BugZero Checklist:

[x] [Contributor] Classify the bug:

Bug classification

Source of bug:

[ ] 1a. Result of the original design (eg. a case wasn't considered)
[x] 1b. Mistake during implementation
[ ] 1c. Backend bug
[ ] 1z. Other:

Where bug was reported:

[x] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
[ ] 2b. Reported on staging (eg. found during regression or PR testing)
[ ] 2d. Reported on a PR
[ ] 2z. Other:

Who reported the bug:

[ ] 3a. Expensify user
[ ] 3b. Expensify employee
[ ] 3c. Contributor
[ ] 3d. QA
[x] 3z. Other: Applause Internal Team

[x] [Contributor] 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: No PR directly introduced the bug. This happens because the current validation allows empty names in the Name column during CSV upload.

[ ] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:

[x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal
Test:

Navigate to Settings Workspace Categories .
Click on the three-dot menu  and select Import a Spreadsheet .
Use a spreadsheet file where one column contains one or more empty cells (e.g., the attached example file).
Select the column with empty values as the Name  field.
Choose another column as the Required  field.
Click Submit  and verify that the following error message appears:
Oops! The field ("Name") contains one or more empty values. Please review and try again.
Update the Name  field to a column that does not contain empty values and retry the process.
Confirm that the categories are imported successfully without any errors.

Do we agree 👍 or 👎

Test GH: https://github.com/Expensify/Expensify/issues/451500

Contributor: @abzokhattab paid $250 via Upwork (Offer link)
Reviewer: @brunovjk owed $250 via Upwork (Offer link)

authors: IuliiaHerets brunovjk huult kadiealexander neil-marcellini


text:
Allow uploading attachments of any file type

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/11189
Tests & QA
Must be tested with https://github.com/Expensify/Web-Expensify/pull/35274.

Test uploading of files that are outside of this list: jpg, jpeg, png, gif, pdf, html, txt, rtf, doc, docx, htm, tiff, tif, xml, mp3, mp4, mov
Test uploading a .heic image. (Google heic sample image)
Make sure files are uploaded properly.

PR Review Checklist

Contributor (PR Author) Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] iOS / native
[x] Android / native
[x] iOS / Safari
[x] Android / Chrome
[x] MacOS / Chrome
[x] MacOS / Desktop

[x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product was added in all src/languages/* files
[x] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

[ ] I have verified the author checklist is complete (all boxes are checked off).
[ ] I verified the correct issue is linked in the ### Fixed Issues section above
[ ] I verified testing steps are clear and they cover the changes made in this PR

[ ] I verified the steps for local testing are in the Tests section
[ ] I verified the steps for Staging and/or Production testing are in the QA steps section
[ ] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[ ] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)

[ ] I checked that screenshots or videos are included for tests on all platforms
[ ] I verified tests pass on all platforms & I tested again on:

[ ] iOS / native
[ ] Android / native
[ ] iOS / Safari
[ ] Android / Chrome
[ ] MacOS / Chrome
[ ] MacOS / Desktop

[ ] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[ ] I verified proper code patterns were followed (see Reviewing the code)

[ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
[ ] I verified that comments were added to code that is not self explanatory
[ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[ ] I verified any copy / text shown in the product was added in all src/languages/* files
[ ] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
[ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[ ] I verified the JSDocs style guidelines (in STYLE.md) were followed

[ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[ ] I verified that this PR follows the guidelines as stated in the Review Guidelines
[ ] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[ ] If a new component is created I verified that:

[ ] A similar component doesn't exist in the codebase
[ ] All props are defined accurately and each prop has a /** comment above it */
[ ] Any functional components have the displayName property
[ ] The file is named correctly
[ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[ ] The only data being stored in the state is data necessary for rendering and nothing else
[ ] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[ ] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[ ] All JSX used for rendering exists in the render method
[ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[ ] If a new CSS style is added I verified that:

[ ] A similar style doesn't already exist
[ ] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

[ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[ ] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

QA Steps

Screenshots

Web
https://user-images.githubusercontent.com/9680864/197581936-37b677d1-a265-4b69-ab5a-f5d287c4be4e.mov
Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Please let me when ready for review. It seems you are still making changes.

@parasharrajat this can only be tested internally, as it required a branch from a private repo to be checked out first.

Ok. Then I am unassigning. Thanks.

[x] I have verified the author checklist is complete (all boxes are checked off).
[x] I verified the correct issue is linked in the ### Fixed Issues section above
[x] I verified testing steps are clear and they cover the changes made in this PR

[x] I verified the steps for local testing are in the Tests section
[x] I verified the steps for Staging and/or Production testing are in the QA steps section
[x] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)

[x] I checked that screenshots or videos are included for tests on all platforms
[x] I verified tests pass on all platforms & I tested again on:

[x] iOS / native
[x] Android / native
[x] iOS / Safari
[x] Android / Chrome
[x] MacOS / Chrome
[x] MacOS / Desktop

[x] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[x] I verified proper code patterns were followed (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product was added in all src/languages/* files
[x] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I verified that this PR follows the guidelines as stated in the Review Guidelines
[x] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] Any functional components have the displayName property
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Just merged and approved https://github.com/Expensify/Web-Expensify/pull/35274, once that's on prod will circle back here!

Checklists are OK.

:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

🚀 Deployed to staging by @youssef-lr in version: 1.2.23-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

Desktop working as expected.

Tested well on Web

This is not working on native apps. Myself and @Justicea83 see the same error when uploading from native Android and iOS. I was thinking about not reverting, but this current behaviour is worse than the current behaviour on prod -- which shows a nice error message (see below)
I'm going to revert this PR
Staging
Android

iOS

Production

Hey @jasperhuangg @youssef-lr it looks like this wasn't tested on Android or iOS, did we miss this? Or did we just forget to upload the screenshots?

@Julesssss do you mind sending me the PDF file that's not working? I'm able to upload PDFs in IOS native.

🚀 Deployed to production by @Julesssss in version: 1.2.23-9 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

authors: Julesssss Justicea83 OSBotify jasperhuangg parasharrajat youssef-lr


text:
explanation for changes in #14728

Signed-off-by: Prince Mendiratta prince.mendi@gmail.com
Details

Adds a comment explaining the changes made in #14728 as a comment.
Tests

N/A

[x] Verify that no errors appear in the JS console

Offline tests

N/A
QA Steps

N/A

[x] Verify that no errors appear in the JS console

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
[x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

[x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos
N/A

@stitesexpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Reviewer Checklist

[ ] I have verified the author checklist is complete (all boxes are checked off).
[ ] I verified the correct issue is linked in the ### Fixed Issues section above
[ ] I verified testing steps are clear and they cover the changes made in this PR

[ ] I verified the steps for local testing are in the Tests section
[ ] I verified the steps for Staging and/or Production testing are in the QA steps section
[ ] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[ ] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)

[ ] I checked that screenshots or videos are included for tests on all platforms
[ ] I included screenshots or videos for tests on all platforms
[ ] I verified tests pass on all platforms & I tested again on:

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

[ ] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[ ] I verified proper code patterns were followed (see Reviewing the code)

[ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
[ ] I verified that comments were added to code that is not self explanatory
[ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[ ] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
[ ] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[ ] I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[ ] I verified the JSDocs style guidelines (in STYLE.md) were followed

[ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[ ] I verified that this PR follows the guidelines as stated in the Review Guidelines
[ ] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[ ] If a new component is created I verified that:

[ ] A similar component doesn't exist in the codebase
[ ] All props are defined accurately and each prop has a /** comment above it */
[ ] The file is named correctly
[ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[ ] The only data being stored in the state is data necessary for rendering and nothing else
[ ] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[ ] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[ ] All JSX used for rendering exists in the render method
[ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[ ] If any new file was added I verified that:

[ ] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[ ] If a new CSS style is added I verified that:

[ ] A similar style doesn't already exist
[ ] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

[ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[ ] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[ ] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.2.67-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.67-7 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

authors: MelvinBot OSBotify Prince-Mendiratta stitesExpensify