-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 2023-03-13] [$4000] The details view is broken for the room that is inaccessible to the user #14513
Comments
Triggered auto assignment to @sakluger ( |
This seems like a very specific edge-case issue. I'm asking in Slack if other people think we should fix it. |
Yeah, can't really think of any reason why someone would get themselves into this situation. But if you are on a |
Alright, for the sake of consistency, let's get this one fixed. Adding the |
Job added to Upwork: https://www.upwork.com/jobs/~01fa0bb1360012489a |
Current assignee @sakluger is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav ( |
Triggered auto assignment to @AndrewGable ( |
ProposalWe should display the NotFound page when the report is not found or accessible. diff --git a/src/pages/ReportDetailsPage.js b/src/pages/ReportDetailsPage.js
index 1b3af0732..9a8a67973 100644
--- a/src/pages/ReportDetailsPage.js
+++ b/src/pages/ReportDetailsPage.js
@@ -23,6 +23,7 @@ import Text from '../components/Text';
import CONST from '../CONST';
import reportPropTypes from './reportPropTypes';
import withReportOrNavigateHome from './home/report/withReportOrNavigateHome';
+import NotFoundPage from './ErrorPage/NotFoundPage';
const propTypes = {
...withLocalizePropTypes,
@@ -106,6 +107,11 @@ class ReportDetailsPage extends Component {
OptionsListUtils.getPersonalDetailsForLogins(participants, this.props.personalDetails),
isMultipleParticipant,
);
+
+ if (this.props.report.errorFields) {
+ return <NotFoundPage />;
+ }
+
return (
<ScreenWrapper>
<HeaderWithCloseButton |
This comment was marked as outdated.
This comment was marked as outdated.
Looks like something related to As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our Feel free to drop a note in #expensify-open-source with any questions. |
Proposal Instead showing "Hmm... it's not here", we can use diff --git a/src/pages/home/report/withReportOrNavigateHome.js b/src/pages/home/report/withReportOrNavigateHome.js
index 5e74f65a0c..f7da82a3b5 100644
--- a/src/pages/home/report/withReportOrNavigateHome.js
+++ b/src/pages/home/report/withReportOrNavigateHome.js
@@ -24,7 +24,8 @@ export default function (WrappedComponent) {
class WithReportOrNavigateHome extends Component {
componentDidMount() {
- if (!_.isEmpty(this.props.report)) {
+ if (!_.isEmpty(this.props.report)
+ && (_.isEmpty(this.props.report.errorFields) || _.isEmpty(this.props.report.errorFields.createChat))) {
return;
}
Navigation.dismissModal(); Screen.Recording.2023-02-02.at.4.50.33.PM.mp4 |
@0xmiroslav @AndrewGable any thoughts on the proposals we have gotten so far? |
ProposalPlease re-state the problem that we are trying to solve in this issue.Direct link access (deep link) to report sub views (such as /details) does not render correctly. What is the root cause of that problem?If you navigate to Onyx merge instructions{
"onyxMethod": "merge",
"key": "report_6618435619127287",
"value": {
"errorFields": {
"createChat": {
"1675614579263380": "Report no longer exists"
}
}
}
} What changes do you think we should make in order to solve the problem?
WithReportOrNavigateHome will also be renamed to WithReportOrNotFound What alternative solutions did you explore? (Optional)None |
ProposalPlease re-state the problem that we are trying to solve in this issue.Access a report url to which you do not have access first, and then access its details url, The detail view looks like broken. What is the root cause of that problem?When we access a report page first, Onyx will create the report object (even if we don't have access to). App/src/pages/home/report/withReportOrNavigateHome.js Lines 27 to 30 in d9a126d
What changes do you think we should make in order to solve the problem?Now, we have aligned the expected result: show the "Hmm...it's not here" view.
Notes: Consider that our navigation is being rewritten, maybe we could also hold this issue for the navigation project. What alternative solutions did you explore? (Optional)I'm not so sure if a HOC is encouraged to fix this issue. So I also tried to rename Update HOC notes (from my original proposal):
click here to see my original proposalProposalThis issue has some relevance to issue #12676, #12428. Option 1: remove the
|
Upwork job price has been updated to $4000 |
Current assignee @sakluger is eligible for the External assigner, not assigning anyone new. |
Current assignees @s77rt and @parasharrajat are eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to @pecanoro ( |
Sorry Rocio for the bump, Andrew is ooo so this assigned another engineer. |
@mountiny did you mean to assign and unassign Rocio? |
@mountiny Can you please remove the |
@s77rt Updated @luacmartins no, as commented above, that was the External label automation which assigned her because Andrew was ooo at the time. |
@AndrewGable, @sakluger, @s77rt, @parasharrajat, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@MelvinBot The PR has been deployed to staging / production 4 days / 5 hours ago. |
My assignment was wrong. Bot marked it internal and assign me while proposals were being reviewed by another C+. |
Looks like the automation didn't work perfectly here, so I'll add some items manually. Payments due:
Merged 4 days after Contributor assignment, so pretty quick, but does not qualify for efficiency bonus. @s77rt I paid you out in Upwork. @adeel0202 @0xmiroslav I send you offers in Upwork, once you accept I can pay out. Regarding regression testing, I don't think we need to add anything since we'll be updating the details view. |
There was a weekend 🙂 |
@sakluger I think timeline bonus applies here as @0xmiroslav clarified those 4 days included the weekend so it's about 2 business days. |
Thanks for clarifying, I added bonuses. Everyone has been paid out now, thanks! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
If the user doesn't have access to the details view they visit, we should show a similar
Hmm...it's not here
page that we show when you try accessing a chat you don't have access to.Actual Result:
The details view is broken
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.2.58-3
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:
Recording.1353.mp4
broken.details.page.mov
Expensify/Expensify Issue URL:
Issue reported by: @adeel0202
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674498377054109
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: