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

fixed the updation anamoly #883

Merged
merged 16 commits into from
Jan 27, 2023
Merged

fixed the updation anamoly #883

merged 16 commits into from
Jan 27, 2023

Conversation

kb-0311
Copy link
Contributor

@kb-0311 kb-0311 commented Jan 22, 2023

What kind of change does this PR introduce?

bugfix
Issue Number:

Fixes #882

Did you add tests for your changes?
Not Yet , Will make another commit for the tests.

Snapshots/Videos:

If relevant, did you update the documentation?

Summary

Now the Mutations only updates the fields specified explicitly in the argument and keeps the other fields as is.

Does this PR introduce a breaking change?
No.

Other information

Have you read the contributing guide?

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2023

Codecov Report

Merging #883 (b6aafd5) into develop (650f038) will increase coverage by 1.81%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop     #883      +/-   ##
===========================================
+ Coverage    81.28%   83.10%   +1.81%     
===========================================
  Files          168      168              
  Lines        10087    10095       +8     
  Branches       858      897      +39     
===========================================
+ Hits          8199     8389     +190     
+ Misses        1886     1705     -181     
+ Partials         2        1       -1     
Impacted Files Coverage Δ
src/resolvers/Mutation/updateUserProfile.ts 100.00% <100.00%> (+30.76%) ⬆️
src/utilities/uploadImage.ts 18.18% <0.00%> (+1.81%) ⬆️
src/resolvers/Mutation/createEventProject.ts 100.00% <0.00%> (+100.00%) ⬆️
src/resolvers/Mutation/updateEventProject.ts 100.00% <0.00%> (+100.00%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SiddheshKukade
Copy link
Member

Looks like the issue is not assigned to you and you've made a PR without confirmation. Your PR is being closed due to this behaviour.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 22, 2023

@SiddheshKukade Sorry for not mentioning it in the issue that I opened .

I did ask for confirmation in the talwa-api slack channel though and had gotten approval for both the pr and opening the issue.

Please could you reopen it ?

@palisadoes palisadoes reopened this Jan 22, 2023
Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

Please add tests to make sure this doesn't happen again.
Please make sure that the test coverage for the src/lib/resolvers/Mutation/updateUserProfile.ts file is 100%

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 23, 2023

@palisadoes Added the tests for my code However -

  1. to get it to 100 percent test coverage I will also need to mock image file upload, How exactly should I approach that ?
  2. Or if I don't mock the file upload should I just mock the test with hardcoded return arbitrary values for the upload result obj?

@palisadoes
Copy link
Contributor

@palisadoes Added the tests for my code However -

1. to get it to 100 percent test coverage I will also need to mock image file upload, How exactly should I approach that ?

2. Or if I don't mock the file upload should I just mock the test with hardcoded return arbitrary values for the upload result obj?

Could this help?: https://gist.github.com/josephhanson/372b44f93472f9c5a2d025d40e7bb4cc

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 24, 2023

Could this help?: https://gist.github.com/josephhanson/372b44f93472f9c5a2d025d40e7bb4cc

Probably not considering that the upload image function destructures the createReadStream object from the uploaded file so we have to provide an actual stream-readable file to test it thoroughly. So testing with a mock function like that would give an undefined error during destructuring.

const { createReadStream, filename } = await newImageFile;

In my opinion, we have to use some sort of an actual placeholder image file in the codebase exclusively for testing the image upload function by reading that file.

And we probably need a separate PR for that too because the image upload function utility itself has not been tested yet and that's why all the tests which have not reached full coverage is because of lack of test cases for upload and delete image functions.

Can I open up a new issue for the testing upload image function?

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 24, 2023

For now, I think it's best to merge this PR and focus on the file uploading in a separate PR since a lot of resolvers use that function.

kb-0311 and others added 6 commits January 25, 2023 08:26
Bumps [ua-parser-js](https://github.com/faisalman/ua-parser-js) from 0.7.32 to 0.7.33.
- [Release notes](https://github.com/faisalman/ua-parser-js/releases)
- [Changelog](https://github.com/faisalman/ua-parser-js/blob/master/changelog.md)
- [Commits](faisalman/ua-parser-js@0.7.32...0.7.33)

---
updated-dependencies:
- dependency-name: ua-parser-js
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* updated INSTALLATION.md

* refactored and reorganized files/folders for better structure

* updated test coverage included directories and remove unused package copyfiles

* added @types/cors and fixed lint error
@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 25, 2023

I am so sorry for the update notifications you must be getting because of this PR . because of complex conflicts the merge editor was not allowing me to edit it on the merge editor but rn seems all the conflicts are gone now.

Any update regarding this?

@palisadoes
Copy link
Contributor

The code coverage level for the repo has declined for this PR. Please write tests for the file you edited so that coverage reaches 100%

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 25, 2023

The code coverage level for the repo has declined for this PR. Please write tests for the file you edited so that coverage reaches 100%

Thats weird after 90f8580984dcf1474440dd03ba6c34fa91471cdb commit the code coverage in my local system did increase but the same change is not reflected in the codecov on github yet for some reason .

I am really sorry but I still don't know or even have a reference about how I am going to test the uploadImage function as I mentioned here. .

I would love all the help I canget from more experienced talawa contributors.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 25, 2023

@palisadoes Sir, New commit increased the code coverage a bit.
To get it all the way up to 100% though also seems like a PR for this issue -
#559
The purpose of this PR was only to fix the bug and write tests for the code I wrote.
As to the remaining uncovered portion of the file, as I mentioned previously needs additional care -

@palisadoes
Copy link
Contributor

Are you willing to work on #559?

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 25, 2023

Are you willing to work on #559?

Of course 👍
I would love to complete the work on this test file.

@palisadoes
Copy link
Contributor

@SiddheshKukade Please review

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 26, 2023

@palisadoes I just realised making another PR for #559 before this one is closed can cause git conflicts.

SO now I just added the changes that will increase the code coverage for tests/resolvers/Mutation/updateUserFix to 100% in the latest commit.

This PR also closes #559.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 26, 2023

I guess the latest commit rn should be good enough to close both those issues.

Copy link
Member

@noman2002 noman2002 left a comment

Choose a reason for hiding this comment

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

Please try to get the code coverage to 100% for updateUserProfile.ts and uploadImage.ts

@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 26, 2023

Please try to get the code coverage to 100% for updateUserProfile.ts and uploadImage.ts
Hello, again.

  1. The commit b6aafd5 (latest commit) has code coverage 100 percent for the file updateUserProfile.ts but the ci/cd codecov on github has not yet processed the commit for some reason and is showing result for ba69d8b . I hope someone looks into that.
  2. the test file for uploadImage.ts does not exist nor was test for that requiredin this PR.The only reason it is showing up in codecoverage is because it is a mocked function in updateUserProfile.spec.ts to increase code cov for that file. Basically its pseudo coverage. So I don't really think we need to cover it to 100% rn in this PR.
  3. I don't know why the updateEventProject file is even showing up in the codecov on github since it was Commited and closed in some other merged PR which I made.

@palisadoes palisadoes merged commit ee36914 into PalisadoesFoundation:develop Jan 27, 2023
@kb-0311
Copy link
Contributor Author

kb-0311 commented Jan 27, 2023

@palisadoes you can safely close #559 also as I promised The coverage for updateUserProfile.ts is now 100% also.

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.

Bug: User Updation Anomoly
6 participants