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 fetch post by org #2414

Merged
merged 28 commits into from
Apr 7, 2024

Conversation

AVtheking
Copy link
Contributor

@AVtheking AVtheking commented Mar 9, 2024

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

Recording.2024-03-15.003418.mp4

Copy link

github-actions bot commented Mar 9, 2024

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

🎯 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.

Copy link

codecov bot commented Mar 9, 2024

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 ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2414   +/-   ##
========================================
  Coverage    96.25%   96.25%           
========================================
  Files          152      152           
  Lines         7524     7578   +54     
========================================
+ Hits          7242     7294   +52     
- Misses         282      284    +2     

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

@Cioppolo14
Copy link
Contributor

@AVtheking Please fix the failing codecov tests.

AVtheking and others added 4 commits March 10, 2024 11:38
@AVtheking
Copy link
Contributor Author

@Cioppolo14 I have fixed failing codecov tests.

@palisadoes
Copy link
Contributor

Tests are failing

AVtheking and others added 2 commits March 11, 2024 23:15
@AVtheking
Copy link
Contributor Author

I have fixed failing test ,please assign reviewers .

lib/services/post_service.dart Outdated Show resolved Hide resolved
lib/services/post_service.dart Outdated Show resolved Hide resolved
AVtheking and others added 3 commits March 12, 2024 19:54
@AVtheking
Copy link
Contributor Author

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

@noman2002
Copy link
Member

@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
Copy link
Contributor Author

@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?

@AVtheking
Copy link
Contributor Author

@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

AVtheking and others added 2 commits March 20, 2024 15:37
@palisadoes
Copy link
Contributor

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. PARENT ISSUE: Fix the usage of userType talawa-api#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
Copy link
Member

@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.

@AVtheking AVtheking requested a review from palisadoes as a code owner March 25, 2024 11:19
@Azad99-9
Copy link
Contributor

@AVtheking please check the app is failing to build.

@AVtheking
Copy link
Contributor Author

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

@Azad99-9
Copy link
Contributor

Have you downgraded any packages ?

@AVtheking
Copy link
Contributor Author

No just merged with latest upstream

@Azad99-9
Copy link
Contributor

Azad99-9 commented Mar 26, 2024

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.

@palisadoes palisadoes removed their request for review March 27, 2024 00:20
@Azad99-9
Copy link
Contributor

Azad99-9 commented Apr 2, 2024

@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
Copy link
Contributor

Azad99-9 commented Apr 2, 2024

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
Copy link
Contributor Author

@Azad99-9 Thanks ,I would try this.

AVtheking and others added 3 commits April 3, 2024 19:59
fix
fix
@AVtheking AVtheking requested a review from noman2002 April 3, 2024 14:43
@AVtheking
Copy link
Contributor Author

please review this PR.

@palisadoes palisadoes merged commit 0b4e291 into PalisadoesFoundation:develop Apr 7, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change of query post by organization
6 participants