[Regression test steps needed] Pdf attachment view shows incorrect background color

[kavimuru]

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

---

Action Performed:

1. Open any chat.
2. Select a pdf attachment. Once selected, pdf preview should open.
   (Notice the background of the pdf view is different from rest of the modal.)

Expected Result:

Background color should be same throughout the modal.

Actual Result:

Background color is different.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.35-0
Reproducible in staging?: y
Reproducible in production?: y
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/205203061-edef27a3-c034-4f00-a3ec-f9cc199b6727.mov

Expensify/Expensify Issue URL:
Issue reported by: @allroundexperts
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669929777779999

View all open jobs on GitHub

[melvin-bot]

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

[situchan]

Proposal|

https://github.com/Expensify/App/blob/main/src/styles/themes/default.js#L51

-   modalBackground: colors.greenBorders,
+   modalBackground: colors.greenAppBackground,

[melvin-bot]

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

[aldo-expensify]

The "different color in the PDF background" has always been the case, light theme:

@shawnborton do you think we should just close?

[shawnborton]

Oh that's a good point... we probably got rid of the white background in light mode so that it would be easier to distinguish white pages from the background. I still think this would be a nice change for dark mode (to use the app's dark BG color) but we should make sure we don't use white when we re-introduce light mode.

[aldo-expensify]

hehe, I just noticed that @Gonals already implemented this here: #13270

[situchan]

hehe, I just noticed that @Gonals already implemented this here: #13270

yes, I am happy my solution was helpful and merged

[allroundexperts]

Do I get any reporting bonus here? @aldo-expensify @michaelhaxhiu?

[michaelhaxhiu]

Yes @allroundexperts

[allroundexperts]

@michaelhaxhiu I actually proposed the solution with the bug even before @situchan.
https://expensify.slack.com/archives/C049HHMV9SM/p1669929777779999

[michaelhaxhiu]

Hmm I need a moment to review the guidelines as I was OOO for 1.5 weeks. I may be mistaken. Please hang tight.

[michaelhaxhiu]

Ok this is a little confusing here, but I'm gonna list out what I think is most fair.

1. $250 bug reporting bonus for @allroundexperts.

Please apply to this job = https://www.upwork.com/jobs/~0179869026709a9a4a

2. $1000 payout for C+ review for @thesahindia (this is the defacto process mentioned in #expensify-open-source here)

Please apply to this job = https://www.upwork.com/jobs/~01d79087d557540258

3. No payment will be given for the solution, as this was resolved by @Gonals as an internal fix

• @allroundexperts while you made a proposal 8 days ago here, it wasn't approved or posted to this GH in time to inspire the fix that @Gonals sent.
• @situchan while you made a proposal 6 days ago here, it was before the issue was labeled as external and thus it wasn't technically available for external hire:

Note: Issues that have not had the External label applied have not yet been approved for implementation. This means, if you propose a solution to an issue without the External label (which you are allowed to do) it is possible that the issue will be fixed internally. If the External label has not yet been applied, Expensify has the right to use your proposal to fix said issue, without providing compensation for your solution. This process covers the very rare instance where we need or want to fix an issue internally.

This section of the contributing.md guidance has more context.

I realize this is a little grey and I am sorta bummed that you both spent time on it, and are not going to be compensated. But this is our policy for a reason. With that said I think you will both benefit from an upcoming change we are making in this GH - #13395 - tl;dr we are going to "lock" newly created issues to make it clear they aren't available to contributors until marked external.

[michaelhaxhiu]

Try once more please, updated the link.

[situchan]

Thanks for confirmation @michaelhaxhiu.
I hope this rule would be applied to all issues.
I thought I would be eligible because I have seen that on other issues.
i.e. #13203 was internal issue, same case as this one but external contributor was paid out (#13203 (comment))

And @allroundexperts's solution is not the same as what @Gonals fixed but my solution is exactly the same

[michaelhaxhiu]

And @allroundexperts's solution is not the same as what @Gonals fixed but my solution is exactly the same

That's fair, but to be honest the fix is incredibly small (updating a single function in 1 line) and thus it's not as though there was much wiggle room to implement it differently. I think we agree this was a very simple fix?

And further, this line is relevant to highlight again:

Expensify has the right to use your proposal to fix said issue, without providing compensation for your solution.

[michaelhaxhiu]

I would encourage you to stick to applying for jobs with the External and Help Wanted labels, that are not on hold. This is a link to view them easily!

[situchan]

It's pretty simple fix and I am fine with no compensation here but I'd suggest to apply that rule to all the issues so we don't need to look for any issues not marked as External.
I think the reason why contributors submit proposals to non-external issues is that:

• Nowadays, contributors find it difficult to get the external job assigned because many are looking for issues real time and they submit proposals as soon as GH created.
• They already know the risk of no compensation but sometimes they are assigned after switching internal->external or their solutions are picked up and get compensated for internal issues. (i.e. if they submit 5 proposals to non-external issues as soon as they are created, they are compensated for at least 2 issues).

[michaelhaxhiu]

Ah I see. Well the good news is it would be a rule applied to all issues, and should help avoid confusion. In the future we will likely open the scope of work more than it is now (as right now we are laser focused on fixing bugs in New Dot and everything else is in a freeze). Stay tuned for more!

[michaelhaxhiu]

PR was deployed to production yesterday

[michaelhaxhiu]

I'll pay out (1) the bug reporting bonus and (2) the C+ review fee in 2 days (Dec 21)

@allroundexperts can you accept the job offer in Upwork btw?

[thesahindia]

@michaelhaxhiu, it's ready for the payment.

[allroundexperts]

@michaelhaxhiu Applied!

[melvin-bot]

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

[melvin-bot]

@michaelhaxhiu, @Gonals Eep! 4 days overdue now. Issues have feelings too...

[mallenexpensify]

Paid @thesahindia $1000 for C+
Paid @allroundexperts $250 for reporting

leaving this open, I think regression test steps are needed @michaelhaxhiu @Gonals

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

• The PR that introduced the bug has been identified. Link to the PR: N/A - this was always this way.
• 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:
• 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 - here
• A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[melvin-bot]

@michaelhaxhiu, @Gonals Still overdue 6 days?! Let's take care of this!

[melvin-bot]

@michaelhaxhiu, @Gonals 10 days overdue. I'm getting more depressed than Marvin.

[mallenexpensify]

Regression test steps update
Update Compose box - Attachments (PDF) in Testrail

Add
4. Verify the background color around the image is all the same color which is dark green, #051c09

Does anyone know if this was also happening with images? If so we can update this too

Compose box - Attachments (images) in Testrail
Add
9. Verify the background color around the image is all the same color

Dropped in #bug-zero

[michaelhaxhiu]

Thanks for the assist MattA. I had a lot of GHs to get through the past 2 days. Just getting around to this now.

[michaelhaxhiu]

@mallenexpensify did you handle the final checkbox for the regression test (to ensure it's added to Applause QAs test steps)? Just wanna double check if I need to help finalize this to move things forward:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[michaelhaxhiu]

@Gonals do you agree with what @aldo-expensify mentioned in his comment above, btw?

The "different color in the PDF background" has always been the case, light theme:

Not that I think he's wrong 😺 but more so want to ensure you and I are in agreement so I can check the first 1-2 boxes:

• The PR that introduced the bug has been identified. Link to the PR:
• 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:

[mallenexpensify]

https://github.com/Expensify/Expensify/issues/252932
Here it is, got confirmation yesterday but didn't get too and I wasn't clear in the Slack thread

[michaelhaxhiu]

All set in terms of the checklist, closing this now.