PalisadoesFoundation / talawa

Community Organization Management Software. Click on the link below to see our documentation.
https://docs.talawa.io/
GNU General Public License v3.0
306 stars 435 forks source link

fixed fetch post by org #2414

Closed AVtheking closed 3 months ago

AVtheking commented 4 months ago

What kind of change does this PR introduce? Changed postByOrganization query according to new paginatied query in the api.

Issue Number:

Fixes #2404

Did you add tests for your changes? Yes

Video

https://github.com/PalisadoesFoundation/talawa/assets/132201033/27968561-a35d-44a3-9a24-44aa164deb58

github-actions[bot] commented 4 months ago

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if either of these two conditions occur:

  1. The overall code coverage drops below the target threshold of the repository
  2. Any file in the pull request has code coverage levels below the repository threshold

The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.

Reviewers

Do not assign reviewers. Our Queue Monitors will review your PR and assign them. When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

  1. comments in this PR or
  2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

  1. arbitrators of future discussions with other contributors about the validity of your changes
  2. point of contact for evaluating the validity of your work
  3. person who verifies matching issues by others that should be closed.
  4. person who gives general guidance in fixing your tests

Other

:dart: Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 97.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 96.25%. Comparing base (a0ab335) to head (25c16df).

Files Patch % Lines
...ews/after_auth_screens/feed/organization_feed.dart 96.49% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #2414 +/- ## ======================================== Coverage 96.25% 96.25% ======================================== Files 152 152 Lines 7524 7578 +54 ======================================== + Hits 7242 7294 +52 - Misses 282 284 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Cioppolo14 commented 4 months ago

@AVtheking Please fix the failing codecov tests.

AVtheking commented 3 months ago

@Cioppolo14 I have fixed failing codecov tests.

palisadoes commented 3 months ago

Tests are failing

AVtheking commented 3 months ago

I have fixed failing test ,please assign reviewers .

AVtheking commented 3 months ago

@Ayush0Chaudhary I have made changes requested by you please review.

noman2002 commented 3 months ago

@AVtheking Having a button to move to next page is not a good approach in a feed like structure. Please do it without the buttons.

AVtheking commented 3 months ago

@AVtheking Having a button to move to next page is not a good approach in a feed like structure. Please do it without the buttons.

Ok will look into it,are you talking about scrolling approach?

noman2002 commented 3 months ago

please share a updated video.

AVtheking commented 3 months ago

@noman2002 I have shared the updated video But the previous test are failing due to this it is giving me this error image it was not showing this error before this approach .

AVtheking commented 3 months ago

@noman2002 please look into it

AVtheking commented 3 months ago

@noman2002 I have shared the updated v image it was not showing this error before this approach .

@noman2002 When I am writing test to mock scroll behaviour it is giving me this error .But I am nor calling any http request.

Azad99-9 commented 3 months ago

@AVtheking I too have faced the same issue multiple times. From that what I got to know is, the error is not related to http request but it is related to the test you are currently working on. Try to fix your test I mean the scroll behavior one, automatically the http error will disappear. I think it's some kind of glitch or something.

I think this may help.

AVtheking commented 3 months ago

@AVtheking I too have faced the same issue multiple times. From that what I got to know is, the error is not related to http request but it is related to the test you are currently working on. Try to fix your test I mean the scroll behavior one, automatically the http error will disappear. I think it's some kind of glitch or something.

I think this may help.

But what to fix in the test it is giving me error at the moment I implemented drag, Refresh Indicator test which was before passing failed after I used notification listener giving me this same warning

AVtheking commented 3 months ago

@noman2002 this http error is still showing ,can we merge this and open issue write test for this ? As this funcitonality needs to be resolved

palisadoes commented 3 months ago

This is an update on the PR merging freeze:

  1. In the next few hours we will be merging the develop-userTypeFix branch into the develop branch.
  2. The develop-userTypeFix branch was created to fix a long standing design flaw where Admins were Admins of all organizations, not specific ones.
  3. The userType field has been removed from the User collection and it has been replaced by an appUserProfileId field.
    1. This field is null if the user isn’t registered to use the apps. This will help people to add users manually during the event checkin process, or if an Admin wants to manually add someone in the Admin dashboard.
    2. When not null the AppUserProfileID will reference a AppUserProfile collection with App related information such as the organizations for which a user may be an Admin.
    3. The updated schema can be found here https://github.com/PalisadoesFoundation/talawa-api/blob/develop-userTypeFix/schema.graphql
    4. This is the parent issue that we have been using to track progress:
      1. https://github.com/PalisadoesFoundation/talawa-api/issues/1965
  4. This merge will cause some conflicts in your PR.

We decided to do this at the beginning of the weekend to give us all time to adjust PR code and create bug fixes that may arise.

Update your code at or after midnight GMT on the morning of March 23, 2024. (5:30am IST).

If your PRs have already been approved, request a re-review after fixing the conflicts and refactoring to the new AppUserProfileID methodology.

noman2002 commented 3 months ago

@noman2002 this http error is still showing ,can we merge this and open issue write test for this ? As this funcitonality needs to be resolved

Please work with @Azad99-9 and get this fixed in this PR.

Azad99-9 commented 3 months ago

@AVtheking please check the app is failing to build.

AVtheking commented 3 months ago

@Azad99-9 i have tried building it in my local it is working fine,what could be the reason of this fail?

Azad99-9 commented 3 months ago

Have you downgraded any packages ?

AVtheking commented 3 months ago

No just merged with latest upstream

Azad99-9 commented 3 months ago

If you didn't downgrade any packages then It might be a transient bug. Please do a minor commit and try to rebuild the app.

Azad99-9 commented 3 months ago

@AVtheking I have solved the HTTP bug for this issue. please do the below-suggested code changes. You will be good to go.

Azad99-9 commented 3 months ago

The reason for the bug was:

  1. The RefreshIndicator requires scrollNotifications to work properly.
  2. When we return true from onNotification Call back we are essentially making the scroll notifications to be consumed by the NotificationListener itself and not letting it to propagate up the widget tree till the RefreshIndicator. So the RefreshIndicator was never being refreshed in the first place.
  3. By returning false we are letting the scroll notifications to propagate till the RefreshIndicator thus allowing it to refresh smoothly.

As mentioned after doing this the HTTP bug has implicitly vanished as it is just a warning.

I have only solved the HTTP bug and made the test run successfully. You please cover all the remaining lines by writing tests to all possible situations to cover the if blocks in onNotification call back.

Feel free to reach out to me if you need any more futher assistance. Thank you.

AVtheking commented 3 months ago

@Azad99-9 Thanks ,I would try this.

AVtheking commented 3 months ago

please review this PR.