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

Allow App reviewers to test unapproved and private apps #1402

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mdmohsin7
Copy link
Collaborator

@mdmohsin7 mdmohsin7 commented Nov 24, 2024

Adds the ability for admins to add testers and give them access to specific apps to test (that are not yet approved)

@beastoin
Copy link
Collaborator

use backend + dashboard, so that the admin could add themself as a tester.

we should not add the Tester approval ability to the app for now. maybe later.

remove the app changes pls sir 😌

@mdmohsin7
Copy link
Collaborator Author

mdmohsin7 commented Nov 25, 2024

I think you got it wrong haha

This PR adds only the ability to enable and test apps for the testers. They can't approve/reject/review apps from within the omi app. Once the testers test the app, the admin or the tester can approve it from the dashboard

Regarding the adding of testers, this only happens on the backend (currently hardcoded to only my uid), if we wish to store the testers UIDs somewhere in db, we can do that as well.

Every call for testing from the front-end checks if the UID is authorised to perform the action

@beastoin

@beastoin
Copy link
Collaborator

beastoin commented Nov 25, 2024

Tester approval ability

i mean the ability to add/remove the tester. just don't put it in the mobile app for now. so there's no app changes.

so let's go with this way:
1/ add a new api to backend so that the admin could add a new tester to an app / admin ~ the request with admin key.
2/ the tester could see the app just like what the user see when it gets approval(in the future) - they could install it, try it, etc. - so you need to update the backend's logic a bit.

just like how the Testflight / Internal test works - a dead simple ver.

what do you think man ?😌

@mdmohsin7
Copy link
Collaborator Author

I see, I understood how you want it to be. The way I built it, it was only supposed to be for Salman and Neo (and other team members) to be able to just test apps before reviewing them (hence the separate admin area). But what you said also is a nice approach where we can have many reviewers or let anyone be the reviewer.

To your approach, what if we make slight changes such that the app creator can allow others as testers (similar to Android Internal Testing). The creator will simply enter the UIDs or the emails of the people whom he want to allow to test, and then simply those users will have access to that app while it is unapproved and private

@beastoin
Copy link
Collaborator

he app creator can allow others as testers

yes but not now bro. let's enable that feature later, maybe via app, maybe via web.

for now, just put it in the admin dashboard. keep the app simple ;)

@mdmohsin7
Copy link
Collaborator Author

Alright got it. Let me finish it

@mdmohsin7
Copy link
Collaborator Author

Up for review with your suggested changes @beastoin

@beastoin
Copy link
Collaborator

let's improve it a bit @mdmohsin7
1/ any reason why you put the testers at root then testers > uid > apps but not kind of apps > id > testers ?
2/ dont put the verb to api routes, we already have get, post, put, delete,

@mdmohsin7
Copy link
Collaborator Author

1/ I thought about this and I decided to place it at root because when we are dealing with triggers, we will have to iteratively fetch all the apps and then find the ones where the user is a tester. In current implementation, we will know already for what apps the user is a tester, so we eliminate the need for going through or filtering through all the apps to find the apps for which the tester has permission to test

2/ Got it! So I should replace add-access and remove-access to maybe just access with POST and DELETE methods right?

@beastoin

@beastoin
Copy link
Collaborator

1/ > we will have to iteratively fetch all the apps and then find the ones where the user is a tester
which triggers man ? all triggers should be fired based-on the user

2/ don't make me says it twice, do some research, fix it, then ask me to review your new code.

@mdmohsin7 🌚

@mdmohsin7
Copy link
Collaborator Author

1/ Let's take an example of this. Here when get_available_apps is called, we will have to check plugin_data/{app_id}/testers collection for each app to find the apps where the user is a tester. And if we take the current implementation (testers at root), then we simply make one call to testers/{uid} and then fetch all the apps for which the user is a tester
Screenshot 2024-11-29 at 6 05 21 PM

2/ Alright

@mdmohsin7
Copy link
Collaborator Author

Pls check @beastoin

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.

2 participants