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

Fixes MVP - Talawa #2108 #2114

Merged
merged 16 commits into from
Nov 30, 2023
Merged

Conversation

AyushRaghuvanshi
Copy link
Contributor

What kind of change does this PR introduce?

This PR removes all usage of dyanmic linking. Including Sharing Links and creating QR

Issue Number:

Fixes #2108

Did you add tests for your changes?
No, It wasn't relevant.

Summary
#2108 asks to remove three features to make app MVP ready. upon discussion with @Ayush0Chaudhary it was come to conclusion that Chat and notification is already removed.

Hence this PR removes dynamic Linking as well

Does this PR introduce a breaking change?
No

Have you read the contributing guide?
Yes

Copy link

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

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 Nov 18, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (c253cb1) 79.85% compared to head (3ad67aa) 79.90%.

Files Patch % Lines
...views/after_auth_screens/profile/profile_page.dart 0.00% 10 Missing ⚠️
lib/splash_screen.dart 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2114      +/-   ##
===========================================
+ Coverage    79.85%   79.90%   +0.05%     
===========================================
  Files          152      149       -3     
  Lines         7456     6973     -483     
===========================================
- Hits          5954     5572     -382     
+ Misses        1502     1401     -101     

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

@AyushRaghuvanshi
Copy link
Contributor Author

@Ayush0Chaudhary Can you once check if thats all I am required to comment out?

Few Extra changes involves Writing documentations and tests

@Ayush0Chaudhary
Copy link
Contributor

Ayush0Chaudhary commented Nov 19, 2023

We can use QR to produce link that lead to our playstore app.

What do you think @noman2002 ?

@noman2002
Copy link
Member

@Ayush0Chaudhary Yes we can do that but currently we don't have it on playstore.
@palisadoes Your thoughts on this? Should we keep the QR or remove it.

@AyushRaghuvanshi
Copy link
Contributor Author

Hey @noman2002 @Ayush0Chaudhary , If my opinion is welcomed, I would like to also propose that ,

Provided we don't have Playstore link., for the time being we can use our github releases link.

Also we enable invite feature to invite people through social media but rather than using dynamic linking , we craft a message like
Hey, I invite you to install this app and join <Organization Name> .

I think it would be a good alternative for the time being.

@palisadoes
Copy link
Contributor

palisadoes commented Nov 20, 2023

We won't have an App Store presence for the MVP. We don't have a strategy on how to handle incompatibilities between different versions of the mobile app, API and Admin. We'll need one by the middle of next year.

It's best to have a link to the repo. @DMills27 is working on creating an issue that would help finalize where that location would be.

For the purposes of the MVP I'd comment out the QR code code and then reinsert it when we have the definitive link. It'll probably change with each release prior to having an app store presence.

Is there a way to embed a QR code in the repo so that people download the latest to their phone?

@AyushRaghuvanshi
Copy link
Contributor Author

https://github.com/PalisadoesFoundation/talawa/releases/download/automated/app-release.apk

@palisadoes
Something like this?

if we create a QR for this link. It will automatically download the latest build to their mobile phones.

For Example.
image

@palisadoes
Copy link
Contributor

Thanks. I meant could we also embed it in a markdown file on GitHub to make it easier for the development community?

README.md for example, but do so in an automated way so that the QR code in the master branch and develop branches download the appropriate APK.

@AyushRaghuvanshi
Copy link
Contributor Author

I think we can do that by creating two readme files for each branch.

Creating a single Readme file and adding a placeholder is mentioned in this issue which is basically seems like not implemented as of now.

I think best approach would be to create two readme.md for each branch......

Also what does this mean about this PR? Is it alright?

@palisadoes
Copy link
Contributor

palisadoes commented Nov 23, 2023

@AyushRaghuvanshi Keep the QR code. Link to the APK in a way that matches the correct branch, if that is possible. If it's manual, then the resulting test should verify the branch is correct.

Let us know when that is complete so we can give it a final review before merging.

Is there anything in these files that need to be updated because of the disabled code?

  1. README.md
  2. CONTRIBUTING.md
  3. INSTALLATION.md
  4. Other markdown files?

@palisadoes
Copy link
Contributor

@AyushRaghuvanshi have you commented out any unused packages in pubspec.yaml
We don't want to be receiving unnecessary security alerts

noman2002
noman2002 previously approved these changes Nov 23, 2023
@AyushRaghuvanshi
Copy link
Contributor Author

@Ayush0Chaudhary @noman2002 Don't merge this yet. I am still working on the changes @palisadoes recommended.

@noman2002
Copy link
Member

@AyushRaghuvanshi What are the updates on this one? We need to merge this.

@AyushRaghuvanshi
Copy link
Contributor Author

@noman2002 I have made the package version changes. I was researching a lot if i can automate a process where we get QR code according to the branch.

I think we can set up a .env file as well? and in the installation.md we state that to access QR code you need to add the correct branch release link

@palisadoes How does that sound?

@noman2002
Copy link
Member

@AyushRaghuvanshi I don't think it will work. It will make things more complex. I suggest we can just redirect the qr to master branch. There no need to redirect it to develop branch.
@palisadoes Your thoughts on this ?

@palisadoes
Copy link
Contributor

palisadoes commented Nov 25, 2023

The QR code should be for the main branch. Please make the modification

noman2002
noman2002 previously approved these changes Nov 28, 2023
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.

Looks good to me.
@Ayush0Chaudhary Please have a look.

@palisadoes
Copy link
Contributor

@AyushRaghuvanshi What version of the API were you using to test against? What commit were you working with? @Ayush0Chaudhary is working on the main migration with another PR.

@AyushRaghuvanshi
Copy link
Contributor Author

@palisadoes I have the latest pull of the talawa-api repo.

@palisadoes
Copy link
Contributor

@palisadoes I have the latest pull of the talawa-api repo.

Thanks @AyushRaghuvanshi . Are you also up to date with all merged PRs, especially this one?

@kb-0311 @Ayush0Chaudhary This may greatly assist your cross repo MVP goal and the eventual compatibility check.

@AyushRaghuvanshi
Copy link
Contributor Author

@palisadoes I wasn't. But Now I am with my latest push.

@palisadoes
Copy link
Contributor

Please fix the failing tests.

@palisadoes
Copy link
Contributor

@noman2002 @Ayush0Chaudhary Please review and merge if OK.

@palisadoes palisadoes merged commit 2f271ac into PalisadoesFoundation:develop Nov 30, 2023
6 of 7 checks passed
palisadoes pushed a commit to palisadoes/talawa that referenced this pull request Jan 10, 2024
* Fix:Overflow Error on PostWideget

* Remove:Unused Hive Boxes

* Fix:Responsiveness on Icons

* Removed:dynamic Linking

* Fix:Not Changing unauthorized files

* Added:Documentation

* Removed: Empty test

* Modified:Test File name

* Add:Tests for misses

* Revert: removal of qr code

* Fix:Splash bug

* Fix:Format

* Fix:Failing test
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.

MVP - Talawa
4 participants