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

Remove special casing logic for post-facility import from the "Select a source" modal to avoid weird edge cases #10959

Closed
MisRob opened this issue Jul 11, 2023 · 7 comments

Comments

@MisRob
Copy link
Member

MisRob commented Jul 11, 2023

Observed behavior

Select a source modal is not displayed on the Device Channels page after importing a facility in the setup wizard

Peek 2023-07-11 16-37

Expected behavior

Select a source modal

Screenshot from 2023-07-11 13-36-24

is displayed after importing a facility in the setup wizard as described in this Gherkin scenario:

Scenario: Streamlined content import after importing facility
    Given I have successfully imported a facility during device setup
      And I see *Welcome to Kolibri*
      And I see a message that I should import channels to the device
      And I see a message that reports will not display properly without resources
    When I click *Continue*
      Then I see *Select a source*
      And I see the device that I imported from auto-selected
      And I see *Choose another source*
    When I click *Continue*
    # In case the peer device has only the unlisted channels, make sure  that the device setting to allow peers to see them is checked
    Then I see *Select channels for import*
      And I see that all channels are selected by default
    When I click *Import*
    Then I see the import task in the task manager

User-facing consequences

UX is not as streamlined as intended. It's still possible to import content by using Import button on the Device Channels page though.

Steps to reproduce

  1. Start a first facility
  2. Start a fresh instance of a second facility. In the setup wizard, follow Group learning > Full device > Import all data from an existing learning facility and import data from the first facility.
  3. After you're redirected to the Device Channels page, click Continue on the Welcome modal
  4. Select a source modal doesn't show

Context

Kolibri 0.16.0a19.dev0+git.12.gb3be1335

Comments

This issue might be related to

// Assume that if first facility has non-null 'last_successful_sync'
// field, then it was imported in Setup Wizard.
// This used to determine Select Source workflow to enter into
importedFacility() {
const [facility] = this.$store.state.core.facilities;
if (facility && facility.last_successful_sync !== null) {
return facility;
}
return null;
},
},

I noticed that even though I imported a facility in the setup wizard, in this place facility.last_successful_sync was null, and therefore importedFacility computed returned null which caused other parts of the code to wrongly assume non-imported facility context.

@marcellamaki marcellamaki added P0 - critical Priority: Release blocker or regression P1 - important Priority: High impact on UX and removed P0 - critical Priority: Release blocker or regression labels Jul 18, 2023
@MisRob
Copy link
Member Author

MisRob commented Jul 19, 2023

I investigated more closely and as mentioned above, the main cause is that last_successful_sync is null. It is returned by backend from /api/auth/facility/

[
  {
    "id": "17332f3f554ddb61057f58e6522c0a24",
    "name": "Home Facility for admin1",
    "num_users": 1,
    "num_classrooms": 0,
    "last_successful_sync": null,
    "last_failed_sync": null,
    "dataset": {
      "id": "68b2f1544747e33dd3301f25b7ae62a2",
      "learner_can_edit_username": true,
      "learner_can_edit_name": true,
      "learner_can_edit_password": true,
      "learner_can_sign_up": true,
      "learner_can_delete_account": true,
      "learner_can_login_with_no_password": false,
      "show_download_button_in_learn": true,
      "extra_fields": {
        "facility": {},
        "on_my_own_setup": false,
        "pin_code": ""
      },
      "description": "",
      "location": "",
      "registered": false,
      "preset": "nonformal"
    }
  }
]

In FacilityViewset we get last_successful_sync as

last_successful_sync=Subquery(
# the sync command does a pull, then a push, so if the push succeeded,
# the pull likely did too, which means this should represent when the
# facility was last fully and successfully synced
TransferSession.objects.filter(
push=True,
active=False,
transfer_stage=transfer_stages.CLEANUP,
transfer_stage_status=transfer_statuses.COMPLETED,
filter=transfer_session_dataset_filter,
)
.order_by("-last_activity_timestamp")
.values("last_activity_timestamp")[:1]
)
)

Note push=True filter.

Looking at TransferSessions in kolibri shell

In [2]: from morango.models import TransferSession

In [3]: TransferSession.objects.all().values_list("push", "active", "transfer_stage", "transfer_stage_status", "filter")
Out[3]: <QuerySet [(False, False, 'cleanup', 'completed', '68b2f1544747e33dd3301f25b7ae62a2')]>

it can be seen that push is False (even though from user point of view import in the setup wizard was successful) and therefore this transfer session is filtered out.


  • the facility run from pex file from which I imported in the setup wizard: Kolibri 0.16.0a19.dev0+git.10.gbd4c777e, full device
  • development server: Kolibri 0.16.0a20.dev0+git.33.ga1571b7a, full device

@MisRob MisRob removed their assignment Jul 19, 2023
@rtibbles
Copy link
Member

At some point (I think since this was implemented, as it was done quite a few years ago) we stopped pushing in the import flow, as it was unnecessary - this can be seen here in the peer import task: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/auth/tasks.py#L384

So, I don't think this is a reliable way of telling if a facility has just been imported or not. I can't think off the top of my head a way to actually tell this, so maybe the best thing to do here is to not treat the import case specially, and just let it follow the usual import flow?

@jtamiace any recollection of why we were so keen to show peers that had the same facility to import resources from?

@MisRob MisRob added the TODO: needs decisions Design or specifications are necessary label Jul 19, 2023
@bjester bjester self-assigned this Jul 27, 2023
@bjester
Copy link
Member

bjester commented Jul 27, 2023

It is believed that this caused this bug hunt issue we experienced, where the frontend was repeatedly calling the API to get the facilities. At most that should happen once, but for a channel import it doesn't seem necessary. If we want to keep the functionality of filtering the devices to only those with the same facility, for the post-setup use case, we'll need to compose new vue composables to combine the use cases and allow the behavior mentioned above.

@ozer550
Copy link
Member

ozer550 commented Jan 19, 2024

I recently did a clean Kolibri setup with full facility and navigated to the device Section. After dismissing the Welcome Modal, I saw the following Modal:
image
after confirming with the team, seems like showing the empty source isn't very helpful!

@pcenov
Copy link
Member

pcenov commented Mar 14, 2024

Hi @radinamatic and @marcellamaki, some testing observations:

  1. Installing Kolibri 0.15.12 and going through the the scenario Group learning > Full device > Import all data from an existing learning facility doesn't result in showing the 'Select a source' modal and no channel resources are being auto-imported to the device.
  2. In Kolibri 0.16 after going through Group learning > Full device > Import all data from an existing learning facility I am not seeing neither the 'Welcome modal' nor the 'Select a source' modal at Device > Channels but there are some auto-imported channel resources (tested several times with cleared cache and cookies):
2024-03-14_14-47-28.mp4

Also looking at https://www.figma.com/file/MpCG7r5PULOCJsEvnTSti8/Android-Designs-2022?type=design&node-id=0-1&mode=design I am not seeing any mention of the 'Select a source' modal so it should be further clarified what exactly is the expected behavior in Kolibri 0.16.

Let me know if you need something else investigated here.

@radinamatic
Copy link
Member

Thank you @pcenov !

I'm fairly sure that at some point we did have that feature in 0.15 (hence the Gherkin scenario that inspired the OP). But giving the option to immediately import required resources after importing the facility was imperative in 0.15, and is not a concern anymore in 0.16 with automatic content syncing.

@marcellamaki I suggest we close this issue as won't fix, and the QA team will check the Gherkin scenario to make sure it reflects the changes in the current 0.16 implementation.

@radinamatic radinamatic removed their assignment Mar 14, 2024
@radinamatic radinamatic removed the P1 - important Priority: High impact on UX label Mar 14, 2024
@rtibbles rtibbles changed the title "Select a source" modal is not displayed after importing a facility in the setup wizard Remove special casing logic for post-facility import from the "Select a source" modal to avoid weird edge cases Mar 26, 2024
@rtibbles rtibbles removed the TODO: needs decisions Design or specifications are necessary label Mar 26, 2024
@jredrejo
Copy link
Member

Fixed in #12077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants