Open the correct report when access sub report page by deep link#29739
Conversation
|
@hoangzinh 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
Screenshots/VideosWebScreen.Recording.2023-10-18.at.05.52.36.-.web.mp4Mobile Web - ChromeScreen.Recording.2023-10-18.at.06.06.32.-.android.chrome.movMobile Web - SafariScreen.Recording.2023-10-18.at.22.33.22.-.ios.safari.movDesktopScreen.Recording.2023-10-18.at.06.02.42.-.desktop.moviOSScreen.Recording.2023-10-18.at.22.35.38.-.ios.movAndroidScreen.Recording.2023-10-18.at.22.43.49.-.android.mov |
|
@dukenv0307 It doesn't work in IOS and Android for me. Could you help to check? Screen.Recording.2023-10-18.at.06.08.55.mov |
|
@hoangzinh For native, when we open sub report page, LHN will display and ti's expected. |
|
@dukenv0307 I didn't know it. Can you elaborate on it to help me understand why it's expected? Thanks |
|
@hoangzinh I don't have the context for this but in the past I worked in the PR about deep link it's expected. |
|
could you give that PR link? We need to clear whether it's an expected behavior, otherwise QA team will ask us again when they test this PR. |
|
@hoangzinh Here is the PR #23977. |
|
Let's see the note in the video. |
|
Thanks @dukenv0307 I think we should leave a note in Test section of PR description, this PR is only tested in Web/mWeb. |
|
Updated. |
|
@madmax330 Please help to review the PR when you have a chance. |
|
✋ 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/madmax330 in version: 1.3.88-0 🚀
|
|
This PR caused this regression: #30126 On login, the URL is /r/undefined so with the new method the reportID is returned as 'undefined'. The previous method returned null there |
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.88-11 🚀
|
|
Thanks @thienlnam for a quick fix on it. |
Details
Open the correct report when accessing sub report page by deep link
Fixed Issues
$ #28641
PROPOSAL: #28641 (comment)
Tests
Note: The PR is only tested on Web/Desktop
Offline tests
No offline test
QA Steps
Note: The PR is only tested on Web/Desktop
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screencast.from.17-10-2023.15.48.35.webm
Android: mWeb Chrome
Record_2023-10-17-15-15-38.1.mp4
iOS: Native
Screen.Recording.2023-10-17.at.15.43.20.mov
iOS: mWeb Safari
Screen.Recording.2023-10-17.at.15.34.24.mov
MacOS: Chrome / Safari
Screencast.from.17-10-2023.15.11.50.webm
MacOS: Desktop
desktop1.mov