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

Refactor ProfileId provision process to multiple Activities #4865

Open
KevinGitonga opened this issue Feb 3, 2023 · 11 comments
Open

Refactor ProfileId provision process to multiple Activities #4865

KevinGitonga opened this issue Feb 3, 2023 · 11 comments
Assignees
Labels
enhancement End user-perceivable enhancements. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@KevinGitonga
Copy link
Contributor

KevinGitonga commented Feb 3, 2023

Is your feature request related to a problem? Please describe.

We want to provide ProfileId to all intents across the App in a pattern that will promote long term sustainability of the Oppia App by providing ProfileId in a decoupled manner. This is related to #4762.

Describe the solution you'd like

Pack ProfileId into all Intents so that it can be easily accessed by underlying fragments and child classes.
#5248 introduced a CurrentUserProfileIdIntentDecorator utility to provide methods for adding the ProfileId to an intent, and for extracting it where needed. The PR also refactored a number of screens to use these utility methods.

Describe alternatives you've considered

N/A

Additional context

The pattern requested here was first introduced in CurrentAppScreenNameIntentDecorator.kt to allow the easy access by underlying child classes.

The required decorator, CurrentUserProfileIdIntentDecorator.kt was added in #5248.

The remaining task is to refactor all screens(activities and fragments) so that the profileId is passed/retrieved entirely using this utility throughout the app.

To pass the ProfileId to an intent or bundle, do:

val intent =
IntroActivity.createIntroActivity(activity).apply {
putProtoExtra(IntroActivity.PARAMS_KEY, params)
decorateWithUserProfileId(profileId)
}
fragment.startActivity(intent)

To retrieve ProfileId from an intent or Bundle, do:

val profileId = intent.extractCurrentUserProfileId()

The best approach for this would be to make changes to a small to medium set of screens at a time, including their related tests to keep the PR sizes reasonably small. For every changed fragment, corresponding changes may be required in the test files.

Multiple contributors can work on this issue at the same time.

@BenHenning
Copy link
Member

NB: This is a prerequisite for #4986.

@adhiamboperes adhiamboperes added Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. labels Jun 15, 2023
@seanlip seanlip added enhancement End user-perceivable enhancements. and removed Type: Improvement labels Jun 16, 2023
@adhiamboperes
Copy link
Collaborator

PR #4869 attempts to solve this issue. The PR has merge conflicts and some broken tests that need to be fixed. @KevinGitonga is unable to complete it at the moment so someone else needs to take over and update the PR.

@Akshatkamboj14 Akshatkamboj14 self-assigned this Nov 19, 2023
@adhiamboperes adhiamboperes added the good first issue This item is good for new contributors to make their pull request. label May 15, 2024
BenHenning pushed a commit that referenced this issue Jun 24, 2024
…ity intent extras, and saved instance state over to protos (#5248)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
Fixes #4986 
Move all fragment arguments, activity extras, and saved instance state
bundle usage over to using protos entirely.

Fix part of #4865
Introduce a ProfileID decorator to pack ProfileId into all Intents so
that it can be easily accessed by underlying fragments and child
classes. Some classes already use this utility where it was not easy to
add profileId to the intent proto bundle.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

---------

Co-authored-by: Vishwajith-Shettigar <[email protected]>
@adhiamboperes
Copy link
Collaborator

@tobioyelekan, could you PTAL at this issue?

@adhiamboperes adhiamboperes added Work: Low Solution is clear and broken into good-first-issue-sized chunks. and removed Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. labels Dec 3, 2024
@tobioyelekan
Copy link
Contributor

Sure @adhiamboperes , I will take a look and propose a solution

@tobioyelekan
Copy link
Contributor

Hi @adhiamboperes should I start another PR? since #4869 is very old
If so, I think it's best we break the tasks into multiple PRs, maybe scoping it by folder/feature. For example, classroom folder. wdyt?

@adhiamboperes
Copy link
Collaborator

@tobioyelekan, maybe you can work on only one package eg classroom for the first PR so that we gain familiarity with the required changes and expected PR size. then for following PRs you can have up to 3 packages.

@tobioyelekan
Copy link
Contributor

Hi @adhiamboperes, I can't find the package for model, and can't import the classes in it, the app still runs though
Please help

Screenshot 2024-12-11 at 8 56 55 AM Screenshot 2024-12-11 at 8 57 42 AM

@adhiamboperes
Copy link
Collaborator

@tobioyelekan, the model package exists. It contains protos. You need to clean and rebuild your project and the imports should then work.

@tobioyelekan
Copy link
Contributor

Thank you @adhiamboperes , I have tried this, even invalidate caches and restart, it's still not working.
I'm currently investigating if it's a problem with my IDE

@adhiamboperes
Copy link
Collaborator

@tobioyelekan, could you please move this discussion to the discussions tab? Actually, you can do a search there to see if someone has previously encountered this issue.

@tobioyelekan
Copy link
Contributor

Thanks @adhiamboperes , will check that, for now I've switched back to AS bumble bee, works well there.

adhiamboperes pushed a commit that referenced this issue Dec 16, 2024
…5596)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
### Fixes part of #4865
This is a step towards the whole migration, as proposed we should
address this by each feature to make PRs smaller and easy to review.
- The changes here are only scoped to classroom.
- routes to other features maintain `internalProfileId` until the
destination parameter is migrated to `profileId`

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## Screen record



https://github.com/user-attachments/assets/c474a78f-0331-4b0e-918b-abc07df380c4
subhajitxyz pushed a commit to subhajitxyz/oppia-android that referenced this issue Dec 17, 2024
…nter (oppia#5596)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
### Fixes part of oppia#4865
This is a step towards the whole migration, as proposed we should
address this by each feature to make PRs smaller and easy to review.
- The changes here are only scoped to classroom.
- routes to other features maintain `internalProfileId` until the
destination parameter is migrated to `profileId`

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## Screen record



https://github.com/user-attachments/assets/c474a78f-0331-4b0e-918b-abc07df380c4
subhajitxyz added a commit to subhajitxyz/oppia-android that referenced this issue Dec 17, 2024
adhiamboperes added a commit that referenced this issue Dec 23, 2024
<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
### Fixes part of #4865 

This PR aim to refactor `topic` package to use `ProfileId`
Changes include all topic fragments, presenters, and TopicActivity
Also updated the test classes.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

---------

Co-authored-by: Adhiambo Peres <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
Development

Successfully merging a pull request may close this issue.

6 participants