Fix Source Course Option only to Current Course in Duplication Data #7298
Conversation
bivanalhar
force-pushed
the
bivan/freeze-source-duplication
branch
6 times, most recently
from
2c9ea77
to
d518438
Compare
bivanalhar
force-pushed
the
bivan/freeze-source-duplication
branch
2 times, most recently
from
a750a8b
to
daad1f1
Compare
cysjonathan
force-pushed
the
bivan/freeze-source-duplication
branch
from
a629b24
to
7360577
Compare
| const ExistingCourseDestinationForm = (): JSX.Element => { | ||
| return ( | ||
| <CourseDropdownMenu | ||
| additionalClassName="destination-course-dropdown" |
There was a problem hiding this comment.
Do you really need to create an additional prop for this? I don't see CourseDropdownMenu used anywhere else.
There was a problem hiding this comment.
in the previous version (while still using jsx) this is how they implement it. And after some contemplation, this is more sensible than combining all since this additional class name is only for the testing case (rspec, when they want to trigger the clicking of the dropdown menu) and hence having different purpose than the className defined inside this CourseDropdownMenu
By meaning, let's say we want to create another component that needs this, inside rspec test later, we can differentiate easier between this component and the later-defined component
There was a problem hiding this comment.
In that case, your onChange function on the <Select> component should also remain on this level.
Currently the dispatch is specific to actions.setDestinationCourseId instead of being agnostic.
In terms of rspec, we can alternatively give this <CourseDropdownMenu className="destination-course-dropdown-menu"> and then use .find('.destination-course-dropdown select') which will find the descendant tag.
client/app/bundles/course/duplication/components/IndentedCheckbox.tsx
Outdated
Show resolved
Hide resolved
client/app/bundles/course/duplication/components/CourseDropdownMenu.tsx
Outdated
Show resolved
Hide resolved
| @@ -40,7 +39,7 @@ describe('<ObjectDuplication />', () => { | |||
|
|
|||
| await waitFor(() => expect(spy).toHaveBeenCalled()); | |||
|
|
|||
| const data = useAppSelector(selectDuplicationStore); | |||
| const data = store.getState().duplication; | |||
There was a problem hiding this comment.
What's the reason we move to .tsx then regress back to .jsx?
There was a problem hiding this comment.
oh sorry, this one I thought I can also convert from jsx to tsx, but apparently doing so creates more problem (and hence I also understand why the other jstest uses jsx rather than tsx) and hence I convert it back to what it's used to be..
There was a problem hiding this comment.
create more problem
what problem?
There was a problem hiding this comment.
sorry.. the conversion towards tsx previously failed because I used the wrong function (I should maintained store.getState().duplication instead of using the other one, to ensure that the content of the store is updated when the duplication value is fetched)..
However, do note that at this point, the type of data is not defined yet since duplicationStore is still in js format (reformatting it to ts will take too much time, and hence I am not keen to do that at this point). Hence, I just re-cast data into the types that is needed for this testing, that is destinationCourses and materialComponents.
There was a problem hiding this comment.
Don't leave migration to TS half-done. How much time is too much time?
- only allow duplication from current course - remove all APIs used only in choosing source course - change page title in duplication page to include course title
bivanalhar
force-pushed
the
bivan/freeze-source-duplication
branch
3 times, most recently
from
e9b5545
to
42035a2
Compare
bivanalhar
requested review from
cysjonathan
and removed request for
ekowidianto
bivanalhar
force-pushed
the
bivan/freeze-source-duplication
branch
3 times, most recently
from
2caa1cc
to
b1ed3a6
Compare
- split multiple components in one file into multiple files
- due to change in the UI for duplication page - source course button needs to be removed
- after migrating to tsx, all values in form reset to initial values - update redux to update the initial values as well
bivanalhar
force-pushed
the
bivan/freeze-source-duplication
branch
from
b1ed3a6
to
2fe03fd
Compare
|
Please refer to #7481 |
Related to issue #7289
Approach
From the Possible Fixes enlisted in the Issue page, we decided to go with the first one, which is only to lock the source course option to the current course. The rationale behind is that since this Duplicate Data is available for all courses, then if user wishes to duplicate something from other course, he/she needs to navigate to that course first then can do the duplication properly
UI Improvement
Since we only restrict the option of Source Course into the current course, there is a slight change in the UI for Duplicate Data page, in which the options for Source Course is removed altogether and the Title of the page includes the Course Title. The comparison is as follows:
Other Changes