-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 E/E #311912] Fix/25680: Assignee does not show on Confirm task page #25756
[HOLD E/E #311912] Fix/25680: Assignee does not show on Confirm task page #25756
Conversation
e808e77
to
0120b53
Compare
@rushatgabhane 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] |
@rushatgabhane updated! Please help check my PR |
@rushatgabhane @mountiny It seems we just have a new update on the main branch. Now we can't search for new users in the assignee modal. Now we need to confirm the expected
Expected (need to confirm)
|
@rushatgabhane please help review my PR, thanks! |
@DylanDylann that looks like a regression. I can't think of any reason why we shouldn't be able to assign a task to any user. |
@DylanDylann could you please merge lates |
…n-confirm-task-page
@rushatgabhane merged latest main into my branch |
@DylanDylann for some reason im stuck on the report loading screen when I run this branch. Do you experience the same? |
no. It is fine from my side |
@rushatgabhane @mountiny Currently the PR #25806 is merged to resolve issue #25688 so that we cannot reproduce the issue #25680 |
@DylanDylann I think we can revert that cc @marcaaron @thienlnam @DylanDylann Will your solution work even after signing out and signing back in when the other user exists but they have display name set (in that case the personal details returned wont have login/email) |
These steps:
Following the above steps, my solution works properly. Please help re-test. Thanks @mountiny @rushatgabhane |
this user, do they have display name set up? |
No. The email is displayed |
can you try with the dispaly name set, then you wont have the email locally available |
@mountiny Follow these steps it works properly: (with the canInviteUser : true) Steps:
|
@mountiny Bump |
@rushatgabhane could you come back to this one please? |
@DylanDylann #25806 (comment) well it seems like creating a new user through this flow is not supported in BE if I understood this correctly, @rushatgabhane to confirm |
@rushatgabhane please help confirm it. Thanks |
@mountiny Bump |
Sorry i am mainly busy with the deliverables for the conferences now, we will come back to this later as it seems a bit more intertwined |
Yeah it looks like it's not allowed by the back-end |
This should still allow optimistic assignees that you haven't chatted with before, but will still fail on contacts that don't exist at all - this is kind of how it was before which is fine |
@thienlnam @mountiny @rushatgabhane So should we revert this PR ? |
Let's verify whether reverting it reintroduces the deploy blocker it was created to resolve? |
Yes, fine, except the part about the app crashing when you try to do it and they don't exist - that was introduced and is not fine 😄 |
@mountiny @marcaaron @thienlnam This PR, we don't allow users to assign to assignees that you haven't chatted with before and don't exist at all. But as the comment from @thienlnam
we only don't allow users to assign to assignees who don't exist at all. So, I think we should revert this PR and create another PR to fix this issue as the correct expected. Please correct me if I am wrong |
Ok, yeah I think the expected behaviour and the easiest for now until this is updated across stack is just to allow assinging people you have in the personalDetails @thienlnam @marcaaron do you agree? |
Yeah indeed, though a recent PR introduced that regression not the original PR that allowed this I have a follow up issue to handle assignees that don't exist yet in the BE. We can just address this after that follow up issue is done |
So we should hold this PR until the BE change is done |
@thienlnam could you put this on hold for the backend PR you got in works? |
Details
Assignee does not show on Confirm task page because we don't have their personal details yet
Fixed Issues
$ #25680
PROPOSAL: #25680 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screencast.from.23-08-2023.18.23.29.webm
Mobile Web - Chrome
test.mp4
Mobile Web - Safari
Screen.Recording.2023-08-23.at.17.58.27.mp4
Desktop
Screen.Recording.2023-08-23.at.18.01.47.mp4
iOS
Screen.Recording.2023-08-23.at.18.25.48.mp4
Android