feat: remove PII from urls #452
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #452 +/- ##
==========================================
+ Coverage 84.93% 85.91% +0.97%
==========================================
Files 185 185
Lines 3862 3868 +6
Branches 962 956 -6
==========================================
+ Hits 3280 3323 +43
+ Misses 564 527 -37
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/ProgramEnrollments/ProgramInspector/ProgramInspector.test.jsx
Outdated
Show resolved
Hide resolved
src/ProgramEnrollments/ProgramInspector/ProgramInspector.test.jsx
Outdated
Show resolved
Hide resolved
Ali-D-Akbar
left a comment
Ali-D-Akbar
left a comment
There was a problem hiding this comment.
Bunch of comments here n there. Looks good otherwise.
| setSsoRecords([]); | ||
| navigate('/programs'); | ||
| } else { | ||
| const newLink = `/programs?edx_user=${ |
There was a problem hiding this comment.
Why did we remove /programs again?
There was a problem hiding this comment.
Earlier, we would navigate to this url. Now, we are only using it to store the "query" (backend api call) that we should make. I have refactored it a little. Hopefully it's clearer now. I also removed the ClickEvent state, as it wasn't being used anywhere.
| }) | ||
| .catch(err => { | ||
| console.error(err); | ||
| setError('An error occured while fetching user id'); |
There was a problem hiding this comment.
Please update in all places.
| jest.spyOn(ssoAndUserApi, 'getEnrollments').mockImplementation(() => Promise.resolve(enrollmentsData)); | ||
| jest.spyOn(ssoAndUserApi, 'getEnterpriseCustomerUsers').mockImplementation(() => Promise.resolve(enterpriseCustomerUsersData)); | ||
|
|
||
| jest.clearAllMocks(); |
There was a problem hiding this comment.
Shouldn't we have this in afterEach?
There was a problem hiding this comment.
I feel like it is fine in both afterEach and beforeEach. Most tests add it in beforeEach too.
src/users/UserPage.test.jsx
Outdated
| jest.spyOn(ssoAndUserApi, 'getSsoRecords').mockImplementation(() => Promise.resolve(ssoRecordsData)); | ||
| jest.spyOn(ssoAndUserApi, 'getLicense').mockImplementation(() => Promise.resolve(licensesData)); | ||
| jest.spyOn(ssoAndUserApi, 'getEntitlements').mockImplementation(() => Promise.resolve(entitlementsData)); | ||
| jest.spyOn(ssoAndUserApi, 'getEnrollments').mockImplementation(() => Promise.resolve(enrollmentsData)); |
There was a problem hiding this comment.
We got a duplicate line here.
There was a problem hiding this comment.
This line is written twice.
jest.spyOn(ssoAndUserApi, 'getEnrollments').mockImplementation(() => Promise.resolve(enrollmentsData));
src/users/UserPage.test.jsx
Outdated
| wrapper = mount(<UserPageWrapper />); | ||
| wrapper.find( | ||
| "input[name='userIdentifier']", | ||
| ).instance().value = 'ANonyMouse'; |
There was a problem hiding this comment.
This may not trigger input change if I'm not wrong... 🤔
There was a problem hiding this comment.
Probably, yes. But we don't need a change event anyways.
There was a problem hiding this comment.
Additionally, when I tried a simulate('onchange'...), the value of the input wasn't changing for some reason, and hence the test was failing. Thus, I took this approach.
| await waitFor(() => { | ||
| expect(mockedNavigator).toHaveBeenCalledWith( | ||
| `/learner_information/?lms_user_id=${UserSummaryData.userData.id}`, | ||
| ); |
There was a problem hiding this comment.
Can we add some assertions to check if the API was called?
There was a problem hiding this comment.
I believe this should call getAllUserData if I'm not wrong 😵💫
PROD-4319
This PR makes the following changes:-
These changes are intended to remove PII like emails and usernames from the urls.