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 #2822 : Add label AppVersionActivity #2905

Merged
merged 13 commits into from
Mar 30, 2021
Merged

Fix #2822 : Add label AppVersionActivity #2905

merged 13 commits into from
Mar 30, 2021

Conversation

E13ctron
Copy link
Contributor

@E13ctron E13ctron commented Mar 14, 2021 •
edited
Loading

Explanation

Fixes #2822 : Add label AppVersionActivity

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@FareesHussain FareesHussain self-assigned this Mar 14, 2021
@FareesHussain FareesHussain self-requested a review March 14, 2021 14:18
FareesHussain
FareesHussain suggested changes Mar 14, 2021
View reviewed changes
Copy link
Contributor

@FareesHussain FareesHussain left a comment

Choose a reason for hiding this comment

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

@Amar-2003 Thanks for the pr, left few comments do have a look.

According to the 4th point in Instructions for making a code change

  • Update the PR description so that it includes Fixes #2822
  • And before commiting make sure that you don't include the auto-generated code by Android-Studio, in your case includes the following files
    • .idea/markdown-navigator-enh.xml
    • .idea/markdown-navigator.xml
    • .idea/misc.xml

.idea/markdown-navigator-enh.xml Outdated Show resolved Hide resolved
.idea/markdown-navigator.xml Outdated Show resolved Hide resolved
.idea/misc.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@E13ctron
Copy link
Contributor Author

Ok fine will do it @FareesHussain

@E13ctron
Copy link
Contributor Author

@Amar-2003 Thanks for the pr, left few comments do have a look.

According to the 4th point in Instructions for making a code change

  • Update the PR description so that it includes Fixes #2822

  • And before commiting make sure that you don't include the auto-generated code by Android-Studio, in your case includes the following files

    • .idea/markdown-navigator-enh.xml
    • .idea/markdown-navigator.xml
    • .idea/misc.xml

Made the changes, please have a look and sorry for the inconvenience

@FareesHussain FareesHussain assigned E13ctron and unassigned E13ctron Mar 14, 2021
FareesHussain
FareesHussain suggested changes Mar 14, 2021
View reviewed changes
Copy link
Contributor

@FareesHussain FareesHussain left a comment

Choose a reason for hiding this comment

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

NIT fix, PTAL

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@prayutsu
Copy link
Contributor

@Amar-2003 Can you also add a test case to verify that the label is actually present?

@rt4914
Copy link
Contributor

rt4914 commented Mar 15, 2021

Will wait for approval from @prayutsu and @FareesHussain first. Thanks @Amar-2003

@rt4914
Copy link
Contributor

rt4914 commented Mar 15, 2021

@Amar-2003 If you check the original issue there is a reference PR mentioned. In that PR you will notice that there is test case which checks this functionality. So please add something similar for this PR too.

FareesHussain
FareesHussain approved these changes Mar 15, 2021
View reviewed changes
Copy link
Contributor

@FareesHussain FareesHussain 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

@E13ctron
Copy link
Contributor Author

I don't know about adding test cases, so i will learn right away and do the neccessary changes

@prayutsu
Copy link
Contributor

@Amar-2003 You can take a look at reference PR #2750.

@E13ctron
Copy link
Contributor Author

E13ctron commented Mar 16, 2021 •
edited
Loading

in file AppVersionActivityTest, line 248 there is some error, should i mind it?? @prayutsu

@E13ctron
Copy link
Contributor Author

in file AppVersionActivityTest, line 248 there is some error, should i mind it?? @prayutsu

I am pushing my changes without considering that, so the line 248 changes to 263

@prayutsu
Copy link
Contributor

@Amar-2003 PTAL, the lint tests are failing, please fix that so that we can review your PR.
Also, please choose a more meaningful branch name in your future PRs(it should not include your name/username and it should consist of only lowercase English letters and different words should be separated by -).

@E13ctron
Copy link
Contributor Author

@Amar-2003 PTAL, the lint tests are failing, please fix that so that we can review your PR.
Also, please choose a more meaningful branch name in your future PRs(it should not include your name/username and it should consist of only lowercase English letters and different words should be separated by -).

The branch name doesn't include my user name.Lint test i will have a look :)

@prayutsu
Copy link
Contributor

@Amar-2003 PTAL, the lint tests are failing, please fix that so that we can review your PR.
Also, please choose a more meaningful branch name in your future PRs(it should not include your name/username and it should consist of only lowercase English letters and different words should be separated by -).

The branch name doesn't include my user name.Lint test i will have a look :)

Ah, I always interpret it wrong, thanks for pointing it out.

prayutsu
prayutsu suggested changes Mar 23, 2021
View reviewed changes
Copy link
Contributor

@prayutsu prayutsu left a comment

Choose a reason for hiding this comment

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

@Amar-2003 Just had a nit suggestion. PTAL.

app/src/sharedTest/java/org/oppia/android/app/administratorcontrols/AppVersionActivityTest.kt Outdated Show resolved Hide resolved
@E13ctron
Copy link
Contributor Author

@prayutsu PTAL

@rt4914 rt4914 assigned rt4914 and prayutsu and unassigned E13ctron Mar 24, 2021
@E13ctron
Copy link
Contributor Author

E13ctron commented Mar 24, 2021 •
edited
Loading

Are there anymore corrections I should do?? @rt4914

prayutsu
prayutsu suggested changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@prayutsu prayutsu left a comment

Choose a reason for hiding this comment

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

Left a comment. @Amar-2003 PTAL.

@prayutsu prayutsu assigned E13ctron and unassigned prayutsu Mar 24, 2021
@E13ctron
Copy link
Contributor Author

@prayutsu PTAL

@E13ctron E13ctron requested a review from prayutsu March 24, 2021 09:59
prayutsu
prayutsu approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@prayutsu prayutsu 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!

@E13ctron
Copy link
Contributor Author

LGTM, Thanks!

Thanks for patiently finding the issues and helping me to get through, this was my first PR to open source :)

@E13ctron
Copy link
Contributor Author

@rt4914 PTAL

rt4914
rt4914 approved these changes Mar 25, 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.

FareesHussain
FareesHussain approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@FareesHussain FareesHussain 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 unassigned rt4914 Mar 25, 2021
@E13ctron
Copy link
Contributor Author

@BenHenning PTAL

BenHenning
BenHenning approved these changes Mar 30, 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 @Amar-2003! This LGTM.

@BenHenning BenHenning merged commit 20dd5d2 into oppia:develop Mar 30, 2021
@E13ctron E13ctron deleted the label-app-version-activity branch March 30, 2021 05:01
E13ctron
E13ctron commented Mar 30, 2021
View reviewed changes
Copy link
Contributor Author

@E13ctron E13ctron left a comment

Choose a reason for hiding this comment

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

good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FareesHussain FareesHussain FareesHussain approved these changes

@prayutsu prayutsu prayutsu approved these changes

@BenHenning BenHenning BenHenning approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[A11y] Add label for AppVersionActivity
5 participants
@E13ctron @prayutsu @rt4914 @BenHenning @FareesHussain