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

Add access revoked dialog #4025

Conversation

karlenDimla
Copy link
Contributor

@karlenDimla karlenDimla commented Dec 19, 2023

Task/Issue URL: https://app.asana.com/0/0/1206140936022690/f

Description

See attached task description

Steps to test this PR

https://app.asana.com/0/0/1206193043882713/f

@karlenDimla
Copy link
Contributor Author

karlenDimla commented Dec 19, 2023

@karlenDimla karlenDimla force-pushed the feature/karl/netp/expired-notif branch from b9f69b1 to afbf704 Compare December 19, 2023 12:43
@karlenDimla karlenDimla force-pushed the feature/karl/netp/expired-notif-dialog branch 5 times, most recently from ecfae07 to 8e7c2ff Compare December 19, 2023 15:26
@karlenDimla karlenDimla marked this pull request as ready for review December 19, 2023 15:26
@karlenDimla karlenDimla requested a review from aitorvs December 19, 2023 15:26
@karlenDimla karlenDimla changed the title WIP on dialog Add access revoked dialog Dec 19, 2023

import com.duckduckgo.navigation.api.GlobalActivityStarter.ActivityParams

sealed class SubscriptionScreens {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcosholgado let me know if you have any comments on this as well since included changes into some subscription stuff

@karlenDimla karlenDimla force-pushed the feature/karl/netp/expired-notif branch from afbf704 to 3247153 Compare December 21, 2023 13:43
return state.asStateFlow()
}

override suspend fun authorize() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we discussed this, we should return NetpAuthorizationStatus in this method for it not to entirely be a side effect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will not fix - will address in https://app.asana.com/0/0/1206170013724653/f

Comment on lines 81 to 85
private fun attemptToAuthorize() {
viewModelScope.launch {
netpSubscriptionManager.authorize()
netpAuthManager.authorize()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this method at all? we can just call that as part of getState() flow in onStart() {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will not fix - will address in https://app.asana.com/0/0/1206170013724653/f

Copy link
Collaborator

@aitorvs aitorvs left a comment

Choose a reason for hiding this comment

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

had left some comments but forgot to approve to unblock if all agreed

@karlenDimla karlenDimla force-pushed the feature/karl/netp/expired-notif-dialog branch from 8e7c2ff to 3cf7c3f Compare December 22, 2023 15:17
@karlenDimla karlenDimla merged commit e297bad into feature/karl/netp/expired-notif Dec 22, 2023
4 checks passed
@karlenDimla karlenDimla deleted the feature/karl/netp/expired-notif-dialog branch December 22, 2023 15:57
karlenDimla added a commit that referenced this pull request Dec 22, 2023
<!--
Note: This checklist is a reminder of our shared engineering
expectations.
The items in Bold are required
If your PR involves UI changes:
1. Upload screenshots or screencasts that illustrate the changes before
/ after
2. Add them under the UI changes section (feel free to add more columns
if needed)
If your PR does not involve UI changes, you can remove the **UI
changes** section

At a minimum, make sure your changes are tested in API 23 and one of the
more recent API levels available.
-->

Task/Issue URL:  https://app.asana.com/0/0/1206140936022690/f

### Description
See attached task description

### Steps to test this PR

https://app.asana.com/0/0/1206193043882713/f
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