Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-03-13] [MEDIUM] Follow-ups to the LHN previews #34411

Closed
5 of 6 tasks
koko57 opened this issue Jan 12, 2024 · 58 comments
Closed
5 of 6 tasks

[HOLD for payment 2024-03-13] [MEDIUM] Follow-ups to the LHN previews #34411

koko57 opened this issue Jan 12, 2024 · 58 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production NewFeature Something to build that is a new item. Task Weekly KSv2

Comments

@koko57
Copy link
Contributor

koko57 commented Jan 12, 2024

Coming from: #30042

Tracking issue listing all of the issues that should be addressed in the follow-ups:

Ideas for other improvements:

  • move getting lastActor / actor display name to a separate function, try to move it out the functions called in getLastMessageTextForReport (create message text and get actor name separately then combine it)
  • getReportPreviewMessage - pass an object with params instead of comma separated params
Issue OwnerCurrent Issue Owner: @adelekennedy
@koko57
Copy link
Contributor Author

koko57 commented Jan 12, 2024

cc @mountiny

@koko57
Copy link
Contributor Author

koko57 commented Jan 17, 2024

working on this one - I've extracted the logic into a separate function, but it's still WIP

@mountiny
Copy link
Contributor

Thanks!

@koko57
Copy link
Contributor Author

koko57 commented Jan 22, 2024

@mountiny the function I extracted still had some problems. I thought that it would be maybe better to divide this refactor into smaller steps. First one - I created a function for getting lastActorName. I also moved the logic for creating alternate text for archived room to getLastMessageTextForReport - I think we should move as much logic for alternateText as possible there.

Let me know what you think about it and if I can open this PR for the review

@koko57
Copy link
Contributor Author

koko57 commented Jan 23, 2024

@mountiny just to make sure it's an expected behaviour (found it on staging, after applying the changes it's still the same):
on the Search Page for archived rooms (workspace chats) we're displaying the reason for the member (pink underline), but a "Workspace" for the admin (blue unedrline). On LHN we're displaying the reason both for the member and the admin. Is that correct?
Screenshot 2024-01-23 at 15 01 12

@mountiny
Copy link
Contributor

Hmm I would say this should be same for user and the admin

@trjExpensify
Copy link
Contributor

In search results, the workspace chat should be:

Admin/other participants

Tom Rhys Jones
Duraflame, Inc.

Member

Duraflame, Inc.
Workspace

@trjExpensify
Copy link
Contributor

@mountiny Nikki reported this here, but there hasn't been an issue created for it yet.

@mountiny
Copy link
Contributor

@trjExpensify So the fix is the other way we should not show the archive reason for any of them, right?

@trjExpensify
Copy link
Contributor

I don't think this is about the archiveReason specifically, that probably just happens to be the last thing in the chat being previewed which is incorrect.

@koko57
Copy link
Contributor Author

koko57 commented Jan 23, 2024

@trjExpensify I can fix it here 🙂

@trjExpensify
Copy link
Contributor

Okay, excellent. I've linked that thread here so we don't create a dupe issue.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Jan 25, 2024
@mountiny
Copy link
Contributor

@koko57 Just wanted to point out this issue which was raised by Pavlo here, lets focus on that one next in this case so we can remove the usage of the originalMessage.amount and .currency

@koko57
Copy link
Contributor Author

koko57 commented Jan 29, 2024

ok! thanks for updating the list in the description!

@greg-schroeder greg-schroeder changed the title Follow-ups to the LHN previews [MEDIUM] Follow-ups to the LHN previews Jan 31, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 31, 2024
@melvin-bot melvin-bot bot changed the title [MEDIUM] Follow-ups to the LHN previews [HOLD for payment 2024-02-07] [MEDIUM] Follow-ups to the LHN previews Jan 31, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 6, 2024
@melvin-bot melvin-bot bot changed the title [MEDIUM] Follow-ups to the LHN previews [HOLD for payment 2024-03-13] [MEDIUM] Follow-ups to the LHN previews Mar 6, 2024
Copy link

melvin-bot bot commented Mar 6, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 6, 2024
Copy link

melvin-bot bot commented Mar 6, 2024

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

If no regressions arise, payment will be issued on 2024-03-13. 🎊

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

  • @koko57 does not require payment (Contractor)

@mountiny
Copy link
Contributor

mountiny commented Mar 7, 2024

Agata is ooo this week, not sure if there is anything left to do here. Going to wait for her to confirm

@mananjadhav
Copy link
Collaborator

@mountiny Can you assign me this issue for the C+ review?

@mountiny
Copy link
Contributor

waiting for Agata to come back from ooo

@koko57
Copy link
Contributor Author

koko57 commented Mar 12, 2024

I agree we can deprioritize the DX improvement. maybe we could separate it in its own issue and get someone else to look into this since you are focused on higher priority tasks

@mountiny yes! we can create a separate issue for that 😊
thanks

@mountiny
Copy link
Contributor

Created a new issue #38220

@mananjadhav Can you please help me with the payment summary here?

@mountiny mountiny added the NewFeature Something to build that is a new item. label Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

Copy link

melvin-bot bot commented Mar 13, 2024

Payment Summary

Upwork Job

  • Reviewer: @mananjadhav owed $500 via NewDot
  • Contributor: @koko57 is from an agency-contributor and not due payment

BugZero Checklist (@adelekennedy)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@mananjadhav
Copy link
Collaborator

@mananjadhav Can you please help me with the payment summary here?

@mountiny This got posted by the bot, is there anything else needed from me here? I can see multiple PRs previously here, and each of them has C+ assigned.

@mananjadhav
Copy link
Collaborator

quick bump here @mountiny. Meanwhile I am requesting my payout on the NewDot. Would be great if you can confirm the summary.

@mountiny
Copy link
Contributor

@mananjadhav could you actually help me list out the PRs?

@mananjadhav
Copy link
Collaborator

@mountiny I can see 4 PRs overall linked with this issue:

@eVoloshchak reviewed the following, not sure if these are paid out. But both of them are merged more than a month ago:

@situchan reviewed #35818, for which they're already paid.

I (@mananjadhav) reviewed #36591

@mountiny
Copy link
Contributor

I believe @eVoloshchak was not paid out but the two PRs were one original and second a regression fix. So I would say the payout would be $500 to @eVoloshchak

For @mananjadhav I assume classic $500 then.

Sounds good?

@mananjadhav
Copy link
Collaborator

Sounds good. @mountiny

@JmillsExpensify
Copy link

$500 approved for @mananjadhav based on summary.

@adelekennedy
Copy link

@eVoloshchak based on the above I sent you a contract on Upwork

@eVoloshchak
Copy link
Contributor

@adelekennedy, thanks! No need for Upwork though, I've just requested the payment via NewDot

@JmillsExpensify
Copy link

$500 approved for @eVoloshchak based on summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production NewFeature Something to build that is a new item. Task Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

7 participants