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.

Sign up for GitHub

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

Jump to bottom

Fix part of #3602 Added label for HomeActivity #3850

Merged
merged 22 commits into from
Nov 17, 2021
Merged

Fix part of #3602 Added label for HomeActivity #3850

merged 22 commits into from
Nov 17, 2021

Conversation

vrajdesai78
Copy link
Contributor

Fix part of #3602 Added label for HomeActivity. I have added label Home to HomeActivity.

Essential Checklist

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

@rt4914
Copy link
Contributor

rt4914 commented Sep 28, 2021

Screenshot 2021-09-28 at 8 03 35 PM

You will need to update the file mentioned in this screenshot.

Basically now that we have a label just remove HomeActivity from exemptions file.

@vrajdesai78
Copy link
Contributor Author

@rt4914 Still Regex pattern validation check is failing

rt4914
rt4914 suggested changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@vrajdesai78 PTAL.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
rt4914
rt4914 suggested changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@vrajdesai78
If you see the sample PR mentioned in the issue it contains a test case too.

You need to add test case similar to testReadingTextSizeActivity_hasCorrectActivityLabel

@vrajdesai78
Copy link
Contributor Author

@rt4914 Can you guide me write a proper test actually I haven't write any test before

@rt4914 rt4914 assigned rt4914 and unassigned vrajdesai78 Oct 5, 2021
@rt4914
Copy link
Contributor

rt4914 commented Oct 5, 2021

@vrajdesai78 please use Assignee section correctly so that I can easily review it. https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section

rt4914
rt4914 suggested changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@vrajdesai78 PTAL at the link that I have added in below comment.

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated
@@ -189,6 +195,7 @@ class HomeActivityTest {
private val longNameInternalProfileId: Int = 3
private lateinit var profileId: ProfileId
private lateinit var profileId1: ProfileId
private val summaryValue = "Medium"
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 not required

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Show resolved Hide resolved
@rt4914 rt4914 assigned vrajdesai78 and unassigned rt4914 Oct 6, 2021
@vrajdesai78
Copy link
Contributor Author

My test case is passing locally
image

@rt4914
Copy link
Contributor

rt4914 commented Oct 18, 2021

@vrajdesai78 please make sure you reply to all the comments.

Also to assign PR to anyone just comment "PTAL @" and oppia-bot will assign it.

rt4914
rt4914 suggested changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@vrajdesai78 PTAL, thanks.

.idea/deploymentTargetDropDown.xml Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved
@vrajdesai78
Copy link
Contributor Author

@rt4914 PTAL

@oppiabot oppiabot bot assigned rt4914 and unassigned vrajdesai78 Oct 19, 2021
@vrajdesai78
Copy link
Contributor Author

@rt4914 PTAL.

@oppiabot oppiabot bot assigned rt4914 and unassigned vrajdesai78 Nov 11, 2021
@oppiabot
Copy link

oppiabot bot commented Nov 11, 2021

Unassigning @vrajdesai78 since a re-review was requested. @vrajdesai78, please make sure you have addressed all review comments. Thanks!

rt4914
rt4914 suggested changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Some of the test cases in HomeActivityTest are failing PTAL.

Screenshot 2021-11-12 at 2 13 01 AM

Also, have a look at Static Lint check failure too.

.idea/runConfigurations.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned vrajdesai78 and unassigned rt4914 Nov 11, 2021
@vrajdesai78
Copy link
Contributor Author

@rt4914 how can I pass unit test which are failing ? Can you guide or provide some resource. I have solved static lint issue

@BenHenning
Copy link
Member

@rt4914 how can I pass unit test which are failing ? Can you guide or provide some resource. I have solved static lint issue

@vrajdesai78 see my comment in HomeActivityTest--it should help you get unblocked.

Also, I noticed that there are a lot of seemingly unrelated changes in this PR. Please revert any changes that aren't needed to address the part of #3602 that this is aiming to solve.

@vrajdesai78
Copy link
Contributor Author

@rt4914 PTAL

@oppiabot oppiabot bot assigned rt4914 and unassigned vrajdesai78 Nov 13, 2021
@oppiabot
Copy link

oppiabot bot commented Nov 13, 2021

Unassigning @vrajdesai78 since a re-review was requested. @vrajdesai78, please make sure you have addressed all review comments. Thanks!

rt4914
rt4914 approved these changes Nov 15, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@rt4914 rt4914 assigned BenHenning and anandwana001 and unassigned rt4914 Nov 15, 2021
BenHenning
BenHenning approved these changes Nov 16, 2021
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @vrajdesai78. LGTM for codeowners.

@BenHenning BenHenning removed their assignment Nov 16, 2021
anandwana001
anandwana001 approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM

@rt4914 rt4914 merged commit 4647538 into oppia:develop Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FareesHussain FareesHussain FareesHussain left review comments

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

@anandwana001 anandwana001 anandwana001 approved these changes

Assignees

@anandwana001 anandwana001

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vrajdesai78 @rt4914 @BenHenning @anandwana001 @FareesHussain