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

Navigation issue in Channels #11272

Merged
merged 5 commits into from
Oct 3, 2023

Conversation

ShivangRawat30
Copy link
Contributor

Summary

Solved the Navigation issue while importing from an attached drive/local network.

References

#11187

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Sorry, something went wrong.

Signed-off-by: shivangrawat30 <[email protected]>
@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: very small labels Sep 19, 2023
@MisRob MisRob added the TODO: needs review Waiting for review label Sep 20, 2023
@MisRob MisRob requested a review from pcenov September 20, 2023 12:23
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Hi @ShivangRawat30, apologies but the expected result mentioned by me in #11187 is incorrect as it states: 'The user should be brought back to Device > Channels'
The actual expected result would be for the user to be brought back to the page they came from. So this should be corrected for all 3 available import options and the scenarios would be:

  1. Import from Kolibri Studio (online) > Select resources - clicking the Back arrow should bring the user back to 'Import from Kolibri Studio' modal
  2. Import from Local network or internet > Select resources - clicking the Back arrow should bring the user back to 'Import from 'Device' modal
  3. Import from Attached drive or memory card > Select resources - clicking the Back arrow should bring the user back to 'Import from 'Drive'' modal

Note that you can actually see the correct navigation flow if you use the browser's Back arrow. Let me know if you have any additional questions and many thanks for your efforts on fixing this!

@ShivangRawat30
Copy link
Contributor Author

Screenshot 2023-09-21 at 3 42 07 PM

@pcenov I think the issue lies in the function goForwardFromSelectImportSourceModal because for LOCAL_DRIVE and PEER_KOLIBRI_SERVER the function should return availableChannelsPageLink() with drive link or the address id. Please correct me If I am wrong.

@radinamatic
Copy link
Member

cc @MisRob ⬆️

@MisRob
Copy link
Member

MisRob commented Sep 25, 2023

@ShivangRawat30 I will have a look at this in the next few days

@rtibbles
Copy link
Member

@ShivangRawat30 - yes, the function you are referencing should be imported into the component you have already updated, and then passed the drive id or the peer id if available to generate the back route.

It may be a little cumbersome, but to be sure of the correct link, you can make the back computed prop completely explicit, checking against each of the cases - similarly to how the app bar title is generated:

https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/device/assets/src/views/SelectContentPage/index.vue#L158

If it's Kolibri Studio, you can just call the function with no arguments to get the link, if it's peer import mode, you should pass the addressId parameter, if it's in import from drive mode, the driveId parameter.

@ShivangRawat30
Copy link
Contributor Author

Hey @rtibbles I am having some trouble getting the drive_Id and address_id from the vue store to use as params for the showAvailableChannelsPage function, please correct me If I am on the wrong path.

Screenshot 2023-09-29 at 5 19 49 PM Screenshot 2023-09-29 at 5 23 17 PM

@rtibbles
Copy link
Member

That looks right - if you are in peerImport mode the selectedPeer should be a non empty object, that should have an id property that you can read from (see here where it is read in the same file: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/device/assets/src/views/SelectContentPage/index.vue#L262)

If you are in localImport mode then selectedDrive should be a non empty object, that should have an id property. (see here: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/device/assets/src/views/SelectContentPage/index.vue#L270)

These then need to be passed with the appropriate key to the options object for the availableChannelsPageLink function: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/device/assets/src/views/ManageContentPage/manageContentLinks.js#L19

So, either addressId or driveId.

Signed-off-by: shivangrawat30 <[email protected]>
@ShivangRawat30
Copy link
Contributor Author

@rtibbles please have a look at the changes. 🙂

Signed-off-by: shivangrawat30 <[email protected]>
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

This looks like the right change to me! @pcenov could you confirm that the issue is fixed?

@ShivangRawat30 looks like there are two linting errors to take care of, removing the unused import of ContentWizardPages and returning something from the backRoute computed prop. I would suggest just returning availableChannelsPageLink() after the if statements to ensure a value is always returned.

@pcenov pcenov self-requested a review October 3, 2023 09:54
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Manual QA passes! Thank you @ShivangRawat30!

ShivangRawat30 and others added 2 commits October 3, 2023 16:34
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Changes make sense, manual QA checks out, we're ready to go! Thanks for the work on this, @ShivangRawat30 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: small SIZE: very small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants