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 #3762 : Story activity chapter summary uses correct description #3811

Merged
merged 17 commits into from
Oct 14, 2021
Merged

Fix #3762 : Story activity chapter summary uses correct description #3811

merged 17 commits into from
Oct 14, 2021

Conversation

darkmat13r
Copy link
Contributor

@darkmat13r darkmat13r commented Sep 17, 2021 •
edited by BenHenning
Loading

Explanation

Fix #3762 : Story activity chapter summary uses correct description

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).

For UI-specific PRs only

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)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

Before vs After

@darkmat13r
Copy link
Contributor Author

darkmat13r commented Sep 17, 2021 •
edited
Loading

@rt4914 PTAL

@rt4914 rt4914 self-assigned this Sep 17, 2021
rt4914
rt4914 suggested changes Sep 21, 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.

@darkmat13r
Some test cases in TopicControllerTest domain layer are failing.

https://github.com/oppia/oppia-android/pull/3811/checks?check_run_id=3634696902

This test is failing testRetrieveStory_validStory_returnsStoryWithChapterSummary

@rt4914 rt4914 assigned darkmat13r and unassigned rt4914 Sep 21, 2021
@darkmat13r
Copy link
Contributor Author

@rt4914 PTAL

@oppiabot oppiabot bot assigned rt4914 and unassigned darkmat13r Sep 24, 2021
@oppiabot
Copy link

oppiabot bot commented Sep 24, 2021

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

@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Sep 24, 2021
@rt4914
Copy link
Contributor

rt4914 commented Sep 24, 2021

@BenHenning @anandwana001 Any idea why the bazel tests are failing? I am unable to understand the reason and also I don't think there is any test case which is failing.

BenHenning
BenHenning requested changes Sep 26, 2021
View reviewed changes
Copy link
Sponsor 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 @darkmat13r. Had one main comment that will lead to other changes. Please send back once it's addressed & CI is passing.

@BenHenning @anandwana001 Any idea why the bazel tests are failing? I am unable to understand the reason and also I don't think there is any test case which is failing.

@rt4914 it's because a textproto file was changed to be inconsistent with its corresponding proto file (which led to the textproto->pb pipeline failing). The error handling could probably be better here, but it's a bit tricky to do.

domain/src/main/assets/test_story_id_0.textproto Show resolved Hide resolved
@BenHenning BenHenning assigned darkmat13r and unassigned BenHenning Sep 26, 2021
@anandwana001 anandwana001 removed their assignment Sep 27, 2021
@anandwana001
Copy link
Contributor

Unassigning myself as not codeowner.
Though, if needed for test assign me back.

@BenHenning
Copy link
Sponsor Member

Closing & reopening to restart CI workflow since it stalled for some reason.

@BenHenning BenHenning closed this Oct 6, 2021
@BenHenning BenHenning reopened this Oct 6, 2021
BenHenning
BenHenning requested changes Oct 6, 2021
View reviewed changes
Copy link
Sponsor 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 @darkmat13r. This looks a lot closer, but there are still failures. PTAL at my comment & CI failures for the next items that need to be resolved.

domain/src/main/assets/test_story_id_0.textproto Show resolved Hide resolved
@BenHenning BenHenning assigned darkmat13r and unassigned BenHenning Oct 6, 2021
@darkmat13r
Copy link
Contributor Author

@BenHenning PTAL

@oppiabot oppiabot bot assigned BenHenning and unassigned darkmat13r Oct 11, 2021
@oppiabot
Copy link

oppiabot bot commented Oct 11, 2021

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

BenHenning
BenHenning approved these changes Oct 12, 2021
View reviewed changes
Copy link
Sponsor 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 @darkmat13r. This LGTM!

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Oct 12, 2021
@BenHenning
Copy link
Sponsor Member

@rt4914 I think you still need to review yet per your earlier request changes.

@oppiabot
Copy link

oppiabot bot commented Oct 12, 2021

Assigning @vinitamurthi for code owner reviews. Thanks!

@oppiabot
Copy link

oppiabot bot commented Oct 13, 2021

Unassigning @vinitamurthi since they have already approved the PR.

rt4914
rt4914 approved these changes Oct 14, 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 merged commit 15669dc into oppia:develop Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@vinitamurthi vinitamurthi vinitamurthi approved these changes

@BenHenning BenHenning BenHenning approved these changes

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

StoryActivity chapter summary uses incorrect description
5 participants
@darkmat13r @rt4914 @anandwana001 @BenHenning @vinitamurthi