Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Hide modal when a token has multiple channels #11686

Merged

Conversation

jredrejo
Copy link
Member

@jredrejo jredrejo commented Jan 3, 2024

Summary

When a token results in more than one channel, modal to insert the token didn't dissappear. This PR fixes it.

References

Closes: #11647
Refers to : #9729

Reviewer guidance

Using the procedure described in #11647 :

  • Test it with a token that only has one channel as Kolibri QA Channel
  • Test it with a token that provides more than one channel as latuk-noriz

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

@jredrejo jredrejo added the TODO: needs review Waiting for review label Jan 3, 2024
@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) DEV: frontend SIZE: very small labels Jan 3, 2024
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.

Thank you @jredrejo, I confirm - the flow with importing channels from a collection is working correctly now.

@pcenov pcenov self-requested a review January 3, 2024 15:00
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.

Sorry I noticed an issue - if I click the 'Import with token' button immediately after having loaded the list with channels I can still see all the options in the modal disabled. See the following video:

2024-01-03_17-02-41.mp4

@marcellamaki marcellamaki self-assigned this Jan 3, 2024
@jredrejo
Copy link
Member Author

jredrejo commented Jan 4, 2024

hi @pcenov

Sorry I noticed an issue - if I click the 'Import with token' button immediately after having loaded the list with channels I can still see all the options in the modal disabled. See the following video:

This new issue should be solved now. Clearly the ability to have multiple channels from a single token had not been tested before. I would not be suprised if you find something else in your tests (I had tested as much as I've been able but I don't have the imagination you have to break things 😉 )

@jredrejo jredrejo requested a review from pcenov January 4, 2024 11:13
@pcenov
Copy link
Member

pcenov commented Jan 4, 2024

Hi @jredrejo the issue that you are fixing here is a regression in 0.16 and is working fine in 0.15 where it's been tested before.
I can see that now if I enter the token for the collection the button Import with token disappears completely which is probably OK but it's different from the 0.15 implementation. So my question is - is there something that's preventing this functionality to work exactly as it was in 0.15, specifically the Import with token button to remain visible?
Notice also that when I click the Select resources button and then I click the back arrow I am no longer at 'Select resources for import' from the collection and have to enter the token again - I can report this as a separate issue if it's not caused by any changes made here.

2024-01-04_14-08-11.mp4

@jredrejo
Copy link
Member Author

jredrejo commented Jan 4, 2024

Hi @jredrejo the issue that you are fixing here is a regression in 0.16 and is working fine in 0.15 where it's been tested before. I can see that now if I enter the token for the collection the button Import with token disappears completely which is probably OK but it's different from the 0.15 implementation. So my question is - is there something that's preventing this functionality to work exactly as it was in 0.15, specifically the Import with token button to remain visible?

Actually I have made it disappear in the last commit because I didn't see the sense of having it available there when you have the list of channels. I must confess that after so long using 0.16.x I don't remember how this worked in 0.15.x. If you click on the token it reappears. Anyway, I can make it visible again if you think it's better.

Notice also that when I click the Select resources button and then I click the back arrow I am no longer at 'Select resources for import' from the collection and have to enter the token again - I can report this as a separate issue if it's not caused by any changes made here.

Yes, I've seen that but I thought this was on purpose, code is designed to do exactly that. If you think that's a problem, fill a new issue. I think it'd be good if the designers take a look to decide what's the best UX.

2024-01-04_14-08-11.mp4

@pcenov
Copy link
Member

pcenov commented Jan 4, 2024

If it's not a problem let's keep the Import with token button visible to keep it same as in 0.15 and also because one can directly click it and import from another channel or collection if they wish to do so.
I'll file a separate issue for the back button behavior.

@jredrejo
Copy link
Member Author

jredrejo commented Jan 4, 2024

If it's not a problem let's keep the Import with token button visible to keep it same as in 0.15 and also because one can directly click it and import from another channel or collection if they wish to do so. I'll file a separate issue for the back button behavior.

👍 restored!

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.

The problems with the modal are solved now! Thanks @jredrejo!

@rtibbles rtibbles merged commit b06f0cc into learningequality:release-v0.16.x Jan 4, 2024
33 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) DEV: frontend SIZE: very small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device > Channels - The 'Enter channel token' modal doesn't disappear when importing a channel collection
4 participants