-
Notifications
You must be signed in to change notification settings - Fork 256
Issue 5449 show license audit and special permissions checks #5563
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
Issue 5449 show license audit and special permissions checks #5563
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few initial thoughts!
| # of setting it as an attribute on our custom Celery class | ||
| app = CeleryApp("contentcuration", task_cls=CeleryTask) | ||
| app.config_from_object(settings.CELERY) | ||
| app.autodiscover_tasks(["contentcuration"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there any reason to include this here? In theory it shouldn't be necessary 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, the page failed to load, which led me to suspect that the tasks weren't being registered. I added app.autodiscover_tasks(["contentcuration"]), and that seemed to resolve the issue.
However, upon further testing, I realized the explicit argument is redundant. When app.autodiscover_tasks() is called without arguments, Celery automatically scans all apps in INSTALLED_APPS. Since contentcuration is already listed there, the default configuration handles it correctly without the manual override
| const names = licenseNames.value; | ||
| if (names.length === 0) return ''; | ||
| if (names.length === 1) return names[0]; | ||
| return names.join(', '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just flagging that this is the same as having licenseNames.value.join(", "), .join will return an empty string if the array is empty, and will return just the first element if it just has one element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Alex, the code is now cleaner and more concise!
| const isLoading = ref(false); | ||
| const error = ref(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we don't actually need these properties. And without them, we don't actually need this to be a composable, since it would only be a util function that formats the license IDs, without needing any state or other reactive behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, removed them!
| const invalidLicenseIds = computed(() => { | ||
| if (!props.invalidLicenses || props.invalidLicenses.length === 0) { | ||
| return []; | ||
| } | ||
| return props.invalidLicenses; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpick, but still want to flag that this could be simplified to be just:
const invalidLicenseIds = computed(() => props.invalidLicenses || []);And with this simplification ⬆️, we can see that the only thing these computed properties are doing is to set a default value, then thi means that instead of creating these invalidLicenseIds and includedLicenseIds, a most straightforward way to handle it is to modify the default value of the props definition to be:
invalidLicenses: {
type: Array,
required: false,
default: () => [],
},
includedLicenses: {
type: Array,
required: false,
default:() => [],
},with the default value set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I used const invalidLicenseIds = computed(() => props.invalidLicenses || []) instead!
| const { | ||
| formattedLicenseNames: invalidLicenseNames, | ||
| isLoading: isLoadingInvalid, | ||
| } = useLicenseNames(invalidLicenseIds); | ||
| const { | ||
| formattedLicenseNames: includedLicenseNames, | ||
| isLoading: isLoadingIncluded, | ||
| } = useLicenseNames(includedLicenseIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we don't necessarily need a composable for this transformation, we could only create a helper function and do something like:
const invalidLicenseNames = computed(() => formatLicenseNames(props.invalidLicenses));
const includedLicenseNames = computed(() => formatLicenseNames(props.includedLicenses));There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, fixed!
| watchEffect(() => { | ||
| const ids = resolvedPermissionIds.value; | ||
|
|
||
| if (ids.length === 0) { | ||
| permissions.value = []; | ||
| return; | ||
| } | ||
| fetchPermissions(ids); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we can use watch instead, as we can explicitly define the dependencies of the watch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Fixed!
| const currentPage = ref(1); | ||
|
|
||
| const nonDistributablePermissions = computed(() => { | ||
| return permissions.value.filter(p => !p.distributable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory all permissions here are not distributable because in the fetch request we filter by distributable: false, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wanted to make sure we got nonDistributablePermissions. But It looks redundant, removed!
| watch(specialPermissions, (newVal) => { | ||
| if (newVal && newVal.length > 0) { | ||
| allSpecialPermissionsChecked.value = false; | ||
| } else { | ||
| allSpecialPermissionsChecked.value = true; | ||
| } | ||
| }, { immediate: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just rely on the @update:allChecked event to set this value instead of adding this watcher, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this watch is redundant, removed!
| if (!licenseAuditIsFinished.value) return false; | ||
| const baseCondition = | ||
| canBeEdited.value && publishedDataIsFinished.value && description.value.length >= 1; | ||
| if (needsReplacementConfirmation.value) { | ||
| return baseCondition && replacementConfirmed.value && allSpecialPermissionsChecked.value; | ||
| } | ||
| return baseCondition && allSpecialPermissionsChecked.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we will be checking allSpecialPermissionsChecked for all cases anyway, then we can just add another if at the beginning of the computed property, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense!
| Supports filtering by IDs and distributable status. | ||
| """ | ||
|
|
||
| by_ids = CharFilter(method="filter_by_ids") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the UUIDInFilter here too!
| const { formattedLicenseNames: invalidLicenseNames, isLoading: isLoadingInvalid } = | ||
| useLicenseNames(invalidLicenseIds); | ||
| const { formattedLicenseNames: includedLicenseNames, isLoading: isLoadingIncluded } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the Figma designs, it seems that we should not be displaying the "Special permissions" as an included license in the license check passed notice here. So, just an additional request -- could you please filter out this license before formatting the license names? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
AlexVelezLl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @taoerman! Found some more things that can be improved. Let me know if you have any questions. Thanks! 👐
| Box, | ||
| }, | ||
| setup(props) { | ||
| const invalidLicenseNames = computed(() => formatLicenseNames(props.invalidLicenses)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For displaying the license names, we should also translate them using this translateConstant function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, Thanks!
| setup(props) { | ||
| const invalidLicenseNames = computed(() => formatLicenseNames(props.invalidLicenses)); | ||
| const includedLicenseNames = computed(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we will need to translate these licenses, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it. I added the logic into useLicenseNames.js for less duplications.
| return props.includedLicenses | ||
| .map(id => { | ||
| const license = findLicense(id); | ||
| return license.license_name; | ||
| }) | ||
| .filter(name => name !== 'Special Permissions') | ||
| .join(', '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty much the same logic we have in the formatLicenseNames function. An idea to prevent this duplicated logic would be to extend the formatLicenseNames function to receive an options object, and if it has an excludes option, it will exclude the licenses specified by this option. And we can potentially manage the common need for translating the licenses in that function, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, I extended the formatLicenseNames function to receive an options object exactly!
| }); | ||
| const hasInvalidLicenses = computed(() => { | ||
| return props.invalidLicenses && props.invalidLicenses.length > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more direct way for null checking is using the optional chaining operator: ?.
props.invalidLicenses?.length > 0There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it!
| if (hasInvalidLicenses.value) { | ||
| return `"${invalidLicenseNames.value}" - ${channelCannotBeDistributed$()} ${fixLicensingBeforeSubmission$()}`; | ||
| } | ||
| return `${includedLicenseNames.value} - ${allLicensesCompatible$()}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always try to avoid concatenating the user-facing text with multiple different translations. Different languages have different ways to separate sentences, which is why we can't just rely on separating by spaces.
For this, for example, we can have two different strings that receive the license names to show as prop, and the translation will be the one in charge of separating the sentences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, Thanks!
| const canBeSubmitted = computed(() => { | ||
| if (!allSpecialPermissionsChecked.value) return false; | ||
| if (isPublishing.value) return false; | ||
| if (hasInvalidLicenses.value) return false; | ||
| if (!licenseAuditIsFinished.value) return false; | ||
| const baseCondition = | ||
| canBeEdited.value && publishedDataIsFinished.value && description.value.length >= 1; | ||
| if (needsReplacementConfirmation.value) { | ||
| return baseCondition && replacementConfirmed.value; | ||
| } | ||
| return baseCondition; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we are adding each time more conditions so that canBeSubmitted is true, we can replace all of this with an array of conditions that needs to be met to return true. This way, we make the conditions to be met more explicit:
const conditions = [
allSpecialPermissionsChecked.value,
!isPublishing.value,
!hasInvalidLicenses.value,
licenseAuditIsFinished.value,
canBeEdited.value,
publishedDataIsFinished.value,
description.value.length >= 1,
];
if (needsReplacementConfirmation.value) {
conditions.push(replacementConfirmed.value);
}
return conditions.every(condition => condition);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, Thanks!
| auditLicenses(id) { | ||
| return client.post(window.Urls.channel_audit_licenses(id)).then(response => { | ||
| return response.data; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to prefer async/await instead.
async auditLicenses(id) {
const response = await client.post(window.Urls.channel_audit_licenses(id));
return response.data;
},There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, Thanks!
|
|
||
| export const AuditedSpecialPermissionsLicense = new APIResource({ | ||
| urlName: 'audited_special_permissions_license', | ||
| fetchCollection(params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, Thanks!
| previousPage: { | ||
| message: 'Previous', | ||
| context: 'Button text to navigate to the previous page in pagination', | ||
| }, | ||
| nextPage: { | ||
| message: 'Next', | ||
| context: 'Button text to navigate to the next page in pagination', | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add an Action suffix for these? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it!
| from contentcuration.viewsets.base import ReadOnlyValuesViewset | ||
|
|
||
|
|
||
| class UUIDInFilter(BaseInFilter, UUIDFilter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a UUIDInFilter defined in from contentcuration.viewsets.common import UUIDInFilter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed Thanks!
8356d59 to
c4f5167
Compare
AlexVelezLl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer! Found a couple of things that can still be improved!
| useLicenseAudit.mockReturnValue({ | ||
| isLoading: ref(false), | ||
| isFinished: ref(true), | ||
| invalidLicenses: ref([]), | ||
| specialPermissions: ref([]), | ||
| includedLicenses: ref([]), | ||
| checkAndTriggerAudit: jest.fn(), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have the mock in the __mocks__ folder, we can safely remove these mockReturnValue!
...bmitToCommunityLibrarySidePanel/composables/__mocks__/useLatestCommunityLibrarySubmission.js
Show resolved
Hide resolved
...onents/sidePanels/SubmitToCommunityLibrarySidePanel/composables/__mocks__/useLicenseAudit.js
Show resolved
Hide resolved
...nents/sidePanels/SubmitToCommunityLibrarySidePanel/composables/__mocks__/usePublishedData.js
Show resolved
Hide resolved
|
|
||
| return constantStrings.$tr(licenseName); | ||
| }) | ||
| .filter(name => name !== null && name !== undefined && name !== '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we can also do just .filter(Boolean) and would have the same desired effect!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, Thanks!
|
|
||
| <div data-test="special-permissions-list"> | ||
| <div class="header-section"> | ||
| <div class="title"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there is a title class in vuetify that is affecting this node. Could you please rename this class to prevent the conflict? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @taoerman! That five refers to the number of pages 😅, but the specs only show 3 per page.
| font-weight: bold; | ||
| } | ||
|
|
||
| .description { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the figma specs, we should have the color of this class set to gray.v_700.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it, Thanks!
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| padding-top: 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove this padding-top and only use the marging-top to match the figma specs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it, Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last change here: I know this isn't in the scope of this PR, but could you please hide the languages and/or categories rendering if we don't have any? It'd be better to not render them instead of rendering the optional text character:
Relatedly, could we make the separation between these two rows (if both are present) 4px instead of 16px? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this folder is starting to get too bloated, could we include these three new components inside a "licenseCheck" (in camel case) folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, Thanks!
AlexVelezLl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really excellent, thanks a lot @taoerman for this incredible PR!! 💯 💯 One step closer! Merging!! 🎉

Summary
Introduces a license audit and special permissions verification workflow to the "Submit to Community Library" side panel.
It includes a new backend AuditedSpecialPermissionsLicenseViewSet to expose permission details and a frontend implementation that triggers a background audit task via useLicenseAudit.
Key UI additions include a LicenseStatus component to display audit results (success/warning) and a SpecialPermissionsList that requires users to explicitly confirm granular before submission is enabled.
References
Fixed #5449
Reviewer guidance
Test the changes manually.