Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

fix: Removed network call to backend server when transitioning from "Member Profile" to "Send Request" screen. #138

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

yashhalgaonkar
Copy link

@yashhalgaonkar yashhalgaonkar commented Aug 26, 2020

Removed network call to backend server when transitioning from "Member Profile" to "Send Request" screen.

Description

There was a redundant network call being made while transitioning from Member Profile Page to Send Request Page to get the current user details. The motivation was to remove that call to save device resources and provide a more seamless service to the user.

I changed the members_page_state.dart to include currentUser of type User in MembersPageSuccess state. This will have all the details of the current user.

I changed members_page_bloc.dart to make all the network calls to get current user details and pass that to MembersPageSuccess object instance whenever it is created.

I changed members_page.dart to pass an instance of MembersPageBloc to MemberProfileScreen.

I changed the member_profile.dart to accept the instance of MemberPageBloc passed to it and use to to get current user details.

Fixes #121

Flutter Channel:

  • I have used the Flutter Beta channel on my local machine

Type of Change:

Delete irrelevant options.

  • Code
  • Quality Assurance

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have tested the code manually on my emulator and device. To reproduce, just go from Member Profile to Send Requesnt Page.

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • Any dependent changes have been published in downstream modules

Removed network call to backend server when transitioning from "Member Profile" to "Send Request" screen.
Copy link

@robotjellyzone robotjellyzone left a comment

Choose a reason for hiding this comment

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

@yashhalgaonkar can you please fill those Description & HOW THIS HAS BEEN Tested sections and also, do check the part of code/quality assurance section as well.

@yashhalgaonkar
Copy link
Author

@robotjellyzone I have filled the Description and Test section.

@techno-disaster
Copy link
Contributor

@yashhalgaonkar Just went through the PR, so basically instead of making the user network call when we click on the send request button like we did earlier, you are making the request when the page loads right?

@yashhalgaonkar
Copy link
Author

@techno-disaster Yeah. I have made network call inside the members_page_bloc.dart whenever an instance of MembersPageSuccess is created.

@techno-disaster
Copy link
Contributor

@techno-disaster Yeah. I have made network call inside the members_page_bloc.dart whenever an instance of MembersPageSuccess is created.

The objective was reducing the extra api call, also the way you have implemented this the currentUser is fetched every time a new page is loaded in pagination, this would affect the UX, can you try and have some workaround this?

@yashhalgaonkar
Copy link
Author

@techno-disaster Yeah. I have made network call inside the members_page_bloc.dart whenever an instance of MembersPageSuccess is created.

The objective was reducing the extra api call, also the way you have implemented this the currentUser is fetched every time a new page is loaded in pagination, this would affect the UX, can you try and have some workaround this?

No. Network calls are not made every time a new page is loaded during pagination. I checked it.

await userRepository.getVerifiedUsers((currentState.users.length ~/ 10) + 1);
final users = await userRepository
.getVerifiedUsers((currentState.users.length ~/ 10) + 1);
final User currentUser = await userRepository.getCurrentUser();
Copy link
Contributor

Choose a reason for hiding this comment

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

@yashhalgaonkar I'm talking about this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the event added when the user scrolls to the end of the page to load more users

Copy link
Author

Choose a reason for hiding this comment

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

Okay. I am looking into this.

Copy link
Author

Choose a reason for hiding this comment

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

Should I use sqflite to store the current user info in local storage? This will make it easy to get current user info in all other parts of app as well, solving the problem once and for all.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, give it a try

Removed the redundant network calls being made to fetch user when the refreshed and when scrolled to the end of the list.
yield MembersPageSuccess(users: users, hasReachedMax: false);
final List<User> users =
await userRepository.getVerifiedUsers(pageNumber);
final User currentUser = await userRepository.getCurrentUser();
Copy link
Author

Choose a reason for hiding this comment

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

This network call will be made when the state is MembersPageInitial. This is a necessary call.

yield users.isEmpty
? currentState.copyWith(hasReachedMax: true)
: MembersPageSuccess(
users: currentState.users + users,
hasReachedMax: false,
currentUser: currentState.currentUser,
Copy link
Author

@yashhalgaonkar yashhalgaonkar Aug 31, 2020

Choose a reason for hiding this comment

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

Here while initializing MembersPageSuccess, we need not make the network call again. So removed it.

Removed the redundant network calls being made to fetch user when the refreshed and when scrolled to the end of the list.
@yashhalgaonkar
Copy link
Author

Now the network call for fetching the currentUser is made only once when state changes from MembersPageInitial state to MembersPageSuccess state, as it should be.

@techno-disaster techno-disaster self-requested a review August 31, 2020 15:22
Copy link
Contributor

@techno-disaster techno-disaster left a comment

Choose a reason for hiding this comment

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

LGTM

@techno-disaster
Copy link
Contributor

@anitab-org/qa-team can someone give this a test? you can find the apk in the github actions artifacts

@techno-disaster techno-disaster changed the title type : Enhancement fix: Removed network call to backend server when transitioning from "Member Profile" to "Send Request" screen. Sep 1, 2020
@Akanksha1212 Akanksha1212 added the Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. label Sep 5, 2020
@rpattath
Copy link
Member

@techno-disaster the PR needs 1 more code review approval before the QA team can test.

@rpattath rpattath added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Sep 16, 2020
@techno-disaster
Copy link
Contributor

@anitab-org/coding-team can someone give this a quick review?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Network call being done in transition from Member Profile to Send Request screen
5 participants