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

Update condition to display new label for content imports #11695

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

LianaHarris360
Copy link
Member

@LianaHarris360 LianaHarris360 commented Jan 4, 2024

Summary

This pull request includes a modification where the string "Resources recently updated" is used instead of the label "New" when importing channel resources on the device to resolve any confusion surrounding to recently imported resources that are considered "new" on the device, but could be imported from older versioned channels.

Before:
labelbefore

After:

afterlabel

References

Fixes #9415

Reviewer guidance

Import from someone over the network, making sure that the person you are importing from has an older version of the channel compared to the one currently installed on your device. The label 'Resources recently updated' should be displayed.


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

@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) DEV: frontend SIZE: small labels Jan 4, 2024
@LianaHarris360 LianaHarris360 marked this pull request as ready for review January 4, 2024 15:56
@LianaHarris360 LianaHarris360 added the TODO: needs review Waiting for review label Jan 4, 2024
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.

Code changes makes perfect sense, needs a manual verification, and also holding off approval as this is targeted for the patch.

@rtibbles
Copy link
Member

rtibbles commented Jan 4, 2024

After consultation with @marcellamaki I am retargeting this to 0.16.0, so this can be merged once manual QA is complete.

@marcellamaki marcellamaki requested review from radinamatic and removed request for ozer550 January 5, 2024 14:30
@marcellamaki
Copy link
Member

@radinamatic @pcenov this is ready for manual QA review. Thank you!

@pcenov
Copy link
Member

pcenov commented Jan 8, 2024

Hi @LianaHarris360 for some reason following the steps from the Reviewer guidance section I am actually seeing the "New" label when importing from an older version of the channel. See the following video:

2024-01-08_11-51-47.mp4

@LianaHarris360
Copy link
Member Author

Hi @pcenov, I noticed that when choosing the option to import from another server, the channel version it is selecting from has the same version as the channel on the device ("Version 19"), although the second server on the right side of the screen lists the channel as "Version 8". Since the version is the same when selecting resources, the New label is displayed to indicate that resources were added or updated until the task is cleared, which seems to be the expected behavior. Perhaps in the future there should be a distinction between a newly added channel/version update and newly added resources for more clarity.

@pcenov
Copy link
Member

pcenov commented Jan 8, 2024

Hi @LianaHarris360 but that just makes the problem bigger than I initially thought. I've just double checked this with a newly created channel the first version of which I imported to the server and the second version of which I imported to the other device.
When I attempt to import something from the server to the device it incorrectly states the version of the channel to be the same as the one on the device. This prevents me from testing the scenario outlined in the 'Reviewer guidance' as it always treats the version that's on the server as being the same as the one on the device... :)

@LianaHarris360
Copy link
Member Author

@pcenov It seems that the local channel version is supposed to be displayed, unless it is not available, because the version of the channel on the local device is being used to display the channel contents locally, not the remote version. From my understanding, the New label should be displayed if the remote channel version is newer or if there are newly added resources, even if those resources come from an older version channel. Also, there was some logic that was backwards that has now been updated, the code should work as intended now.

@pcenov
Copy link
Member

pcenov commented Jan 10, 2024

Thanks @LianaHarris360 I can only say that the label "New" is being always displayed when importing content either from Studio or from an older version of the channel from a local device. When importing from an older channel version on a local device the version of the channel is shown as being the same as the one on my device (which is confusing and incorrect). However if I am directly importing the channel for the first time from the local device then it shows the correct version. Here's a video illustrating what I am doing and seeing:

NewLabel.mp4

Based on that I still can't understand how to test the issue described in #9415 so some guidance on that will be much appreciated.

@LianaHarris360
Copy link
Member Author

Ah, yes, I understand your point. When selecting resources to import, if the displayed channel version always matches the one on the device, then the label "New" will always be displayed after importing resources. I'll have to defer to Richard to provide a more thorough explanation as to why the device's channel version is shown instead of the remote version.

Since the device's channel version being displayed is intentional, perhaps the primary issue is not with the code logic, but rather with the labeling of "New", which may cause confusion among users when added resources are not necessarily more current. cc @rtibbles @marcellamaki @radinamatic

@github-actions github-actions bot added DEV: dev-ops Continuous integration & deployment DEV: renderers HTML5 apps, videos, exercises, etc. DEV: backend Python, databases, networking, filesystem... APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: tools Internal tooling for development SIZE: medium labels Jan 11, 2024
@LianaHarris360 LianaHarris360 changed the base branch from release-v0.16.x to develop January 11, 2024 21:05
@rtibbles rtibbles added TAG: user strings Application text that gets translated and removed DEV: dev-ops Continuous integration & deployment DEV: renderers HTML5 apps, videos, exercises, etc. DEV: backend Python, databases, networking, filesystem... APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) DEV: tools Internal tooling for development SIZE: medium labels Jan 11, 2024
@rtibbles
Copy link
Member

Merging - any further iteration on the strings can happen in string review process for 0.17.

@rtibbles rtibbles merged commit 878d246 into learningequality:develop Jan 11, 2024
31 checks passed
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.) DEV: frontend SIZE: small TAG: user strings Application text that gets translated TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing UI when importing content over a network
4 participants