[BUG]: It is possible to select a skill in the dropbox “select a skill to view a question” when a question is already selected to be edited or a new question is already created.

cam-pinheiro commented 1 week ago

Describe the bug

It is possible to select a skill in the dropbox “select a skill to view a question or create a new question” when a question is already selected to be edited or a new question is already created. It could be confusing because the Manager could miss in which skill is being created or edited a question.

URL of the page where the issue is observed.

https://www.oppiatestserver.org/topic_editor/GJ2rLXRKD5hw#/questions

Steps To Reproduce

Preconditions: user has to be granted manager's access to Fractions

Given the topic Manager is on question editor of the Fraction Topic -Topics and Skills Dashboard When the topic Manager clicks on the dropbox “select a skill…” And select a specific skill And clicks on a specific question to be edited or create a new question Then the question is open And the dropbox is still enabled to select any skill.

Expected Behavior

It is expected not be possible to select other skill in the dropbos, when a question is already selected to be edited or a new question is already created

Screenshots/Videos

select skill dropbox.webm

What device are you using?

Desktop

Operating System

Windows

What browsers are you seeing the problem on?

Chrome, Firefox

Browser version

129.0.6668.90 (Chrome) 130.0.1 (Firefox)

Additional context

No response

Tips for developers

Before addressing the bug, please identify which PR caused the issue (you can follow the steps here). If you identify the PR, comment on the issue with a link to it. If not, mention the commit hash of the oldest commit you saw the bug on (and the month and year it was made in).

Then, please leave a comment with details of the approach that you plan to take to fix the issue (see example).

Note: If this is your first Oppia issue, please make sure to follow our guidelines for choosing an issue and setting things up. You will also need to show a demo of the fix working correctly on your local machine. Thanks!

seanlip commented 4 days ago

Thanks for filing this, @cam-pinheiro. I think a suitable approach would be to replace the dropdown with plain text instead when the user is in "question editing" mode. (Would you agree?)

cam-pinheiro commented 3 days ago

Hi, @seanlip I think it is a good approach. This way we will avoid the manager getting confused when is editing a question.

ujjwalpathaak commented 3 days ago

Hey @cam-pinheiro, Currently I went with disabling the <mat-select> tag instead of replacing drop-down with plain text, but that can be added easily too if needed.

Ref 1 - core/templates/pages/topic-editor-page/questions-tab
Ref 2 - core/templates/components/question-directives/questions-list

I am simply adding a new state - isEditing in Ref 1 which is passed down to <oppia-questions-list> ( Ref 2 ) component and is being manipulated using toggleIsEditing() function ( Ref 2 ).

The dropdown gets disabled or enabled wrt if the editor is opened or not. ( shown in the video )

https://github.com/user-attachments/assets/b063c5c4-3fb3-4ae1-b424-11a0143232b1

If this approach is fine can I takeup this issue?

PS - I will add tests for this behaviour && will give better var names while committing :)

seanlip commented 2 days ago

@ujjwalpathaak Thanks for looking into this! Please replace the dropdown with plaintext or a header as described in the previous comment, and show a video for that. Thanks.

ujjwalpathaak commented 2 days ago

Hey, have a look at this.. lemme know if any more changes are needed? Screen Recording 2024-10-16 at 3.05.14 PM.webm

seanlip commented 2 days ago

Thanks @ujjwalpathaak, I think it looks good. Some notes:

Make the header text smaller -- about the same size as "Difficulty".
Change it to "Editing question for skill: XXX" where XXX is the name of the skill.

I think that should be sufficient. Thanks! I've assigned you -- when can you make a PR?

ujjwalpathaak commented 2 days ago

Sure will keep those in mind. I’ll finalize the code, review for edge cases, and open the pull request as soon as possible.

ujjwalpathaak commented 1 day ago

Hey @seanlip, I was trying to push my changes to my fork to open a PR. I get an error in the tests.

-------------------------------------------
Backend associated test file checks failed.
-------------------------------------------
ERROR:root:third_party/python_libs/_pytest/monkeypatch.py needs an associated backend test file.
third_party/python_libs/numpy/typing/tests/data/pass/multiarray.py needs an associated backend test file.

the files I have changed are -

components/question-directives/questions-list/questions-list.component.html
components/question-directives/questions-list/questions-list.component.ts

pages/topic-editor-page/services/topic-editor-state.service.ts

pages/topic-editor-page/questions-tab/topic-questions-tab.component.ts
pages/topic-editor-page/questions-tab/topic-questions-tab.component.html

the defination for make run_tests.check_backend_associated_tests is - Runs the backend associate tests, verifies that backend changes have corresponding test coverage.

Am I missing something? - Do I have to write tests for my frontend changes too before pushing?

mon4our commented 1 day ago

Hi @ujjwalpathaak, can you share the changes you have made? Third party files are usually not a part of the coverage checks, so we can look further into why the tests are failing for these files. Thanks!

ujjwalpathaak commented 1 day ago

questions-list.component.html just added a class and some css over it.

questions-list.component.ts subscribed to the topicEditorStateService used toggleTopicSkillEditor() to pass boolean values to toggle state.

topic-questions-tab.component.html added some conditional rendering and CSS

topic-questions-tab.component.ts subscribed to the topicEditorStateService and used few of it's methods

topic-editor-state.service.ts added a new state _skillEditorOpened and an EventEmitter to toggle it + 2 getter funcs

oppia / oppia