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

2579 curator portal update #2723

Merged
merged 60 commits into from
Oct 3, 2022
Merged

2579 curator portal update #2723

merged 60 commits into from
Oct 3, 2022

Conversation

maciej-zarzeczny
Copy link
Contributor

@maciej-zarzeczny maciej-zarzeczny commented Jun 10, 2022

This relates to #2579 and #2589

This PR required me to refactor some major parts of the application. Also migrating from mui v4 to v5 required me to make changes to every file that used this library. The same goes for tests (after migrating to cypress v10). I also refactored some of the components from class components to function components in order to make the application more up to date. Unfortunately all those changes resulted in 140+ file changes in the PR.

I had many issues with failing tests in GH CI/CD pipeline. They all work now, however I had to remove Firefox browser from the integration tests GH action. All the tests are passing both on Chrome and Firefox locally when running Cypress but for some unknown reason fail on Firefox (only in CI). I read multiple forum threads were users experienced similar problems with Firefox running in CI in headless mode. The problems on Firefox were that it didn't "see" requests made to api/geocode/suggest?q=France, all my efforts to fix that failed. Strange thing is that requests to the same endpoint but in other tests works just fine. Finally I think that this is some kind of a bug coming from Firefox in headless mode. If you have any ideas what can cause such bugs, please let me know.

@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2022

Codecov Report

Merging #2723 (dc98aa6) into main (75729e6) will increase coverage by 5.74%.
The diff coverage is 50.75%.

@@            Coverage Diff             @@
##             main    #2723      +/-   ##
==========================================
+ Coverage   58.66%   64.41%   +5.74%     
==========================================
  Files         116      166      +50     
  Lines        4609     6058    +1449     
  Branches     1246     1596     +350     
==========================================
+ Hits         2704     3902    +1198     
- Misses       1905     2156     +251     
Impacted Files Coverage Δ
data-serving/data-service/src/controllers/case.ts 82.63% <ø> (ø)
...on/curator-service/api/src/clients/email-client.ts 61.53% <ø> (ø)
...cation/curator-service/api/src/controllers/auth.ts 45.01% <0.00%> (-0.13%) ⬇️
...ation/curator-service/api/src/controllers/cases.ts 48.17% <0.00%> (ø)
...ation/curator-service/api/src/controllers/users.ts 67.92% <ø> (ø)
verification/curator-service/api/src/model/user.ts 66.66% <ø> (ø)
...mponents/AcknowledgmentsPage/EnhancedTableHead.tsx 6.66% <ø> (ø)
.../components/AcknowledgmentsPage/helperFunctions.ts 0.00% <0.00%> (ø)
...ce/ui/src/components/AcknowledgmentsPage/index.tsx 0.00% <ø> (ø)
...ce/ui/src/components/AcknowledgmentsPage/styled.ts 50.00% <ø> (ø)
... and 121 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@maciej-zarzeczny maciej-zarzeczny marked this pull request as ready for review August 26, 2022 10:55
@@ -0,0 +1,14 @@
// import './AcknowledgmentsPage.spec';
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this file do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Cypress v10 there is now way to run all tests at once locally. By uncommenting those imports and running All.spec.ts we get this functionality back. It just makes testing locally easier

@jim-sheldon
Copy link
Collaborator

Excellent work, thank you for doing all of this.
I support disabling Firefox tests. If you did not try changing the FF version, that might help.
If anything I would prefer testing using Safari, since more people use it than FF.

@abhidg
Copy link
Contributor

abhidg commented Sep 13, 2022

Thanks @maciej-zarzeczny! Excellent work, few nits -- otherwise looks great :)

execTimeout: 70000,
requestTimeout: 12000,
responseTimeout: 12000,
projectId: 'hx4khd',
Copy link
Contributor

Choose a reason for hiding this comment

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

is this registered somewhere? where do we get this/change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a project in Cypress dashboard in order to enable recording tests in the CI pipeline. I've just added you to this project, you should see the invite on your email

status: 200,
response: '1.10.1',
}).as('fetchVersion');
cy.intercept('GET', '/version', '1.10.1').as('fetchVersion');
Copy link
Contributor

Choose a reason for hiding this comment

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

this will fail once version updates -- maybe worth dropping this test? or checking for a 1. pattern, since unlikely we are going to 2. anytime soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By intercepting this request we are mocking the response with this string 1.10.1 so this should always return static version string

@@ -149,7 +152,7 @@ export default class CasesController {
);
const result = await users().findOneAndUpdate(
{
_id: new ObjectId(user.id),
_id: new ObjectId(user._id),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that _id is returned from MongoDB, I just made this change to be consistent. I think I saw some typescript errors due to using id instead of _id. I will check that


mockedAxios.get.mockImplementation((url) => {
if (url === '/version') {
return Promise.resolve({ status: 200, data: '1.10.1' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this need to be kept updated with the version? If so, might be worth using a regex match here

Copy link
Contributor Author

@maciej-zarzeczny maciej-zarzeczny Oct 3, 2022

Choose a reason for hiding this comment

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

No, I'm mocking the /version endpoint request with a static string as a response. So this is not connected with the actual version

admin2?: string,
admin1?: string,
country?: string,
latitude?: number | string,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually yes, thank you for catching that

@@ -106,6 +107,8 @@ export default function SignInForm({
try {
const token =
(await recaptchaRef.current.executeAsync()) as string;

console.log(token);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to keep these around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, will delete

googleID: '42',
name: 'Alice Smith',
email: 'foo@bar.com',
roles: ['admin', 'curator'],
Copy link
Contributor

Choose a reason for hiding this comment

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

should the initial state have roles set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only used in the tests. In most cases the user needs higher permissions in order to test admin functionalities. I just added this as the initial state not to duplicate code in the tests

Copy link
Contributor

@OskarKocjan OskarKocjan left a comment

Choose a reason for hiding this comment

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

Amazing work!

@abhidg
Copy link
Contributor

abhidg commented Oct 3, 2022

Thanks @maciej-zarzeczny, this was a huge task and much needed :)

@abhidg abhidg merged commit a05ada4 into main Oct 3, 2022
@abhidg abhidg deleted the 2579-curator-portal-update branch October 3, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants