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

Concept Card Name Broken #2134

Closed
mschanteltc opened this issue Nov 20, 2020 · 7 comments · Fixed by #2980
Closed

Concept Card Name Broken #2134

mschanteltc opened this issue Nov 20, 2020 · 7 comments · Fixed by #2980
Assignees
Labels
Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@mschanteltc
Copy link

A Concept Card link in Lesson Player was presented incorrectly. This occurred in the topic Place Value, story Jaime's Adventures in the Arcade, in Chapter 3: Comparing Numbers. On other occasions where I triggered a Concept Card, this did not occur. It might be because the Concept Card is empty. [Video]

Screenshot_20201120-005220_Oppia

@chrk2205
Copy link
Contributor

@mschanteltc
i would like to work on this but i cannot find the topic in my app!!

@viktoriias
Copy link
Contributor

@mschanteltc Could you point out what is broken?
Are you referring to an error-like looking concept card at 0:11? Or bad HTML rendering in the hint: &quote;concept card&quote;?

@mschanteltc
Copy link
Author

@viktoriias I am referring to the bad HTML rendering &quote;concept card&quote; which I would assume causes the empty concept card. If that does not cause the empty concept card, should I file a separate issue for that?

@viktoriias
Copy link
Contributor

@anandwana001 Do you have an advice on reproducing this issue?

@anandwana001
Copy link
Contributor

@anandwana001 Do you have an advice on reproducing this issue?

@viktoriias this is not available in the current develop app, need to modify json files a bit. Adding @rt4914 for required steps.

@BenHenning BenHenning self-assigned this Jan 7, 2021
@BenHenning
Copy link
Member

Taking this as part of the next alpha release.

@BenHenning
Copy link
Member

Aha, I just fixed this in my local alpha work. I actually thought I caused this--glad to see it fixed.

@BenHenning BenHenning linked a pull request Apr 15, 2021 that will close this issue
BenHenning added a commit that referenced this issue Apr 23, 2021
* Add support for loading proto versions of lessons.

* Upgrade Espresso versions to work around build issues.

Note that I can't actually verify Espresso tests are working with these
versions since the Bazel project doesn't yet have Espresso tests set up.

* Lint fix post-copying from #2927.

* Fix Glide in Bazel.

* Fix concept card name escaping.

Also, refactor HtmlParser to better support/handling different tag types
& prepare for LaTeX SVG image support.

* Copy over test fix from #2927.

* Remove image loading annotation.

This will be added in a later branch in the chain.

* Add caching module to needed test suites.

* Add support for importing text protos as assets.

This actually provides Bazel-only support for converting text protos to
binary and including those as assets so that we can avoid checking in
binary protos to the codebase.

* Add gitignore for Android Studio Bazel plugin output.

* Import textproto versions of existing dev assets.

This imports local conversions of all existing dev assets as text proto
leveraging the Bazel-only conversion system to convert these to binary
protos and include them as assets.

This is a necessary step to both eventually replace JSON assets with
text proto, and to test that the new proto loading pipeline is working
as designed.

* Undo Espresso version change since it breaks Bazel tests.

* Fix tests.

Various attempts to fix existing tests + move some of the domain tests
over to actually using proto when running with Bazel.

App module tests are hanging currently for unknown reasons (only in
Bazel).

* Use correct Espresso core.

3.1.0 causes tests to hang indefinitely when run with Bazel.

* Move coroutine dispatchers.

This moves the coroutine dispatchers to a new threading subpackage. It
also simplifies the TestCoroutineDispatcher interface & implementations
(which required moving runUntilIdle to CoroutineExecutorServiceTest).

The test coroutine annotations were also moved to their own files.

* Refactor testing utilities build graph.

This introduces new Bazel libraries for:
- Test dispatchers
- Fake system/Oppia clocks
- Robolectric dependencies

* Introduce dedicated test for TestCoroutineDispatcher.

This also fixes some threading issues in the dispatcher, and clarifies
some of its API that was previously unclear (and can lead to subtle race
conditions).

* Add tests for TestCoroutineDispatcher.

This required a bunch of refactoring, adding the first examples of
dedicated Bazel test build files, and introducing a new test pattern for
sharing code between different implementations. Further, it refined the
API for TesCoroutineDispatcher & fixed some issues in the
implementations (especially Robolectric).

Tests verified as non-flaky in Robolectric across 1000 runs, and 0.2%
flaky across 1000 runs for Espresso (though that may be fixed; waiting
on a follow-up run to confirm stability).

* Workaround ktlint semicolon issue.

* Lint fixes.

* Fix test post-merge.

* Move CoroutineDispatcher to be part of threading.

* Lint fixes.

* Undo version upgrade.

* Fix broken tests.

* Lint fixes.

* Add a bit more proto loading test coverage.

* Comment fix.

* Lint fixes.

* Address earlier TODO.

* Post-merge restructure.

* Post-merge fixes.

This removes unneeded dependencies from PersistentCacheStore, including
Glide.

* Add tests for new tag handlers.

Also, add tests for HtmlParser, and clean up some existing tests.

* Fix broken test build.

* Post-merge lint fixes.

* Lint fixes.

* Address reviewer comment.

* Post-merge fix.

* Fix typo.
@BenHenning BenHenning added the Z-ibt Temporary label for Ben to keep track of issues he's triaged. label Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.

5 participants