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

fix: replace username with full name in the learner record title #294

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

ahtesham-quraish
Copy link
Contributor

@ahtesham-quraish ahtesham-quraish commented Feb 28, 2024

Description: Remove username from learner record title and replace it with Full Name

VAN-1832

Before After
image image

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.37%. Comparing base (9efeceb) to head (22c4c9c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   70.22%   70.37%   +0.14%     
==========================================
  Files          27       27              
  Lines         403      405       +2     
  Branches       85       87       +2     
==========================================
+ Hits          283      285       +2     
  Misses        119      119              
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ahtesham-quraish ahtesham-quraish requested a review from a team February 28, 2024 09:58
@arbrandes
Copy link
Contributor

Why is the username being removed? Has this gotten product review, yet? Apologies if it already has, but I'd like this to be confirmed before we merge it.

@arbrandes arbrandes self-requested a review March 1, 2024 12:38
Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending product review question.

@hurtstotouchfire
Copy link
Member

@arbrandes I was expecting this to be swapping username for full name, which made sense to us since this is a transcript page, basically. Email should be retained because it's used by the credit transfer feature. Open to leaving username if needed, but typically full name is more appropriate for a certificate record.

@arbrandes
Copy link
Contributor

arbrandes commented Mar 4, 2024

@hurtstotouchfire, thanks for the description of the intended behavior. The link in the ticket description is private, unfortunately, so there was no way to understand the reasoning behind the change other than figuring it out from the code.

Still, it's a user-facing change, and we should probably get somebody from Product giving it a 👍🏼. Adding the label in.

@arbrandes arbrandes added the product review PR requires product review before merging label Mar 4, 2024
@hurtstotouchfire
Copy link
Member

Sounds good, thanks

@ahtesham-quraish ahtesham-quraish force-pushed the ahtesham/van-1832 branch 2 times, most recently from 38863cd to 2010a4f Compare March 5, 2024 08:53
@zainab-amir zainab-amir changed the title fix: remove the username from learner record title fix: replace username with full name in the learner record title Mar 20, 2024
@jmakowski1123
Copy link

It makes sense to me to use the full name instead of the username. I'm just wondering if there are any implications on the data pipeline for reporting purposes? Does this change completely remove the association of usernames from learner records? If so, does that impact any future plans for reports that may touch learner records? cc @bmtcril @crathbun428

@zainab-amir
Copy link

Does this change completely remove the association of usernames from learner records? If so, does that impact any future plans for reports that may touch learner records?

This is just a user facing change. The username has been replaced with full name only on the frontend. All the existing functionality that depends on username for learner record has not been touched.

@crathbun428
Copy link

crathbun428 commented Mar 22, 2024

@jmakowski1123 @bmtcril - Capturing this question Brian raised here since I think its an important one. Curious how an instructor may handle identifying a learner that has changed their email address if they have a number of learners with the same name. Will there be a way of identifying those learners in this case?

@jmakowski1123
Copy link

Got it, thanks @zainab-amir .

@crathbun428 Since this is just a frontend change, I'm assuming the data pipeline won't be affected since the user ids aren't being removed from the backend. Something to pass back to Brian, but also to keep in mind down the road if the conversation evolves to removing usernames entirely (which I think is a future state discovery conversation, but not being acted on at the moment).

This has product aproval!

Description: Remove username from learner record title
VAN-1832
Description: Replace username with full name
VAN-1832
@hurtstotouchfire
Copy link
Member

@jmakowski1123 @bmtcril - Capturing this question Brian raised here since I think its an important one. Curious how an instructor may handle identifying a learner that has changed their email address if they have a number of learners with the same name. Will there be a way of identifying those learners in this case?

@crathbun428 This page is a learner-facing transcript. Instructors have no way of navigating to this page. This transcript is used by the learner to track their own progress and can also be shared publicly (like with a CV or sending to a university for transfer credit).

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@ahtesham-quraish ahtesham-quraish merged commit 4445fe3 into master Apr 4, 2024
8 checks passed
@ahtesham-quraish ahtesham-quraish deleted the ahtesham/van-1832 branch April 4, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product review PR requires product review before merging
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants