-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: display activity assignment target (M2-7407, M2-7408) #859
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @qiushihe, before I go too deep into your code, I thought I'd share some feedback in testing this PR as I think it may entail some (hopefully minimal) refactoring.
I have many activities, flows, and assignments locally, since I've been working on MI Assign for some time. In the Web App, each activity assignment is listed as a distinct item, including those for in-progress activities and flows.
However in the mobile app, I still only see one instance of each activity/flow, despite the fact many of these activities/flows have several assignments.
Web | Mobile |
---|---|
Being able support increased granularity of activites/flows, including in-progress activities/flows – now tracking the assignment as well – was a non-trivial part of the work I did for M2-7405 and M2-7406.
I see that this was not made explicit in the Jira ticket, and I apologize for not thinking to mention it. Maybe I thought that since you'd reviewed my work on M2-7405 you'd have clocked that, but I shouldn't have assumed!
Anyway I suspect adding that support will just require building upon your current work. Please let me know if you have any questions around this change!
Oh no. Looking at the AC for M2-7407 this was clearly not stated and is missing entirely. There's actually several differences in the AC between this ticket and the web version, M2-7405 even if there shouldn't be. Left a comment on the ticket for Cari. Let's take the conversation there and perhaps huddle in the morning. |
@farmerpaul I just pushed up all the refactor works I've been working on for the past several days. And I think I got everything covered. So this is now ready for review. Please take a look when you get a chance. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noted a couple remaining issues, which we've discussed via Slack:
- This work isn't behind the feature flag
enableActivityAssign
yet – please make sure functionality is unchanged when this feature flag is turned off. And when the flag is "ON":- When an activity is set to auto-assign = false, and it has no manual assignments, I was still able to see the activity in the list of activities to be able to take. Unassigned activities should in fact be omitted from the list.
- Update activity/flow progress store record key to only have target subject specificity when it's not self report
I know you're working hard on this and I appreciate your persistence! ❤️
ddaefde
to
1f80348
Compare
1f80348
to
2f11741
Compare
Hi @farmerpaul I finally finished all the changes, including the feature flag and those other changes you requested. The feature flag is only checked at 1 place: on line 135 of Please take another look. Thanks! I'm also happy to do more walkthrough/huddle/whatnot too 😄 |
src/widgets/activity-group/model/services/ActivityGroupsBuildManager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a bit more feedback @qiushihe. I reviewed commit by commit to help me digest what the changes were more easily (and prevent my browser from to slowing to crawl during review). But even so, I will admit I only skimmed some of the more bulky commits in order to maintain my sanity, as it's a lot for me to absorb!
I really appreciate the walkthroughs you've given both via huddle and the demo, and I understand the majority of the refactoring changes you've made. But nevertheless I may have missed some details – hopefully QA will catch anything I've missed.
@farmerpaul okay I think I got everything covered. Please take another look. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Billie, great work on all of this!! Thank you for making those changes. I have one last request, but preapproving because I have total faith you'll be able make this change without further approval, and I'd like to speed along getting this into QA.
Another change I made in the web app was to sort the activities/flows according to the AC. The AC says to sort only by subject first name, but I opted to sort both by first name, then last initial.
The original sort method is also inefficient, performing two calls to .filter()
, then two calls to .sort()
. I optimized it to use a single .sort()
call, as well as taking into account target subject. Here's my version, feel free to copy-paste/adapt:
const sort = (eventEntities: EventEntity[]) => {
eventEntities.sort((a: EventEntity, b: EventEntity) => {
const aIsFlow = a.entity.pipelineType === ActivityPipelineType.Flow;
const bIsFlow = b.entity.pipelineType === ActivityPipelineType.Flow;
// Order flows first
if (aIsFlow && !bIsFlow) {
return -1;
} else if (!aIsFlow && bIsFlow) {
return 1;
}
// Then order by entity order
const orderDiff = a.entity.order - b.entity.order;
if (orderDiff !== 0) return orderDiff;
// Then order self-reports first
if (!a.targetSubject) {
return -1;
} else if (!b.targetSubject) {
return 1;
}
// Then order by target subject first name
const firstNameDiff = a.targetSubject.firstName.localeCompare(b.targetSubject.firstName);
if (firstNameDiff !== 0) return firstNameDiff;
// Then order by target subject last name
return a.targetSubject.lastName.localeCompare(b.targetSubject.lastName);
});
};
Then it's just called using:
sort(eventEntities);
📝 Description
🔗 Jira Ticket M2-7407
🔗 Jira Ticket M2-7408
This PR implements the display of activity assignment target, when the target is not the same as the respondent.
📸 Screenshots
Activities Listing Screen
🪤 Peer Testing
full-account-1
activity-1
,activity-2
andactivity-3
full-account-2
to the appletlimited-1
andlimited-2
activity-1
tofull-account-2
withlimited-1
as targetactivity-2
tofull-account-2
withlimited-2
as targetactivity-3
tofull-account-2
withfull-account-2
(i.e. "self") as targetfull-account-2
activity-1
andactivity-2
, but notactivity-3
activity-1
oractivity-2
activity-3
✏️ Notes
This PR also implements parts of: 🔗 Jira Ticket M2-7408
For M2-7408, both the assignment target badge and the assignment target banner are implemented in this PR. However, since the mobile app is currently missing the "welcome" screen, the banner is temporarily placed in
ActivityItem.tsx
, and is hidden. When the related bug ticket is being worked on (https://mindlogger.atlassian.net/browse/M2-7917), the banner should be moved into the correct place on the welcome screen.