-
Notifications
You must be signed in to change notification settings - Fork 237
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
New Plugin Details page #2328
base: main
Are you sure you want to change the base?
New Plugin Details page #2328
Conversation
66e55bd
to
416b421
Compare
The remaining tasks here:
Pagination is probably worth moving to a separate PR/ticket, if so I suggest we keep the plugin list shorter until we have pagination - I added the additional plugins because I was planning to do the 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.
I like where this is going, but I think we should split the changes so the improved install, uninstall and update API is in a separate PR as well as another PR for the improved changes to the internal store of the plugin details.
CHANGELOG.md
Outdated
### Fixes | ||
|
||
- [#2318: Only show Clear search link if there's a search](https://github.com/alphagov/govuk-prototype-kit/pull/2318) | ||
- [#2321: Update hint for create page](https://github.com/alphagov/govuk-prototype-kit/pull/2321) | ||
- [#2317: Using internal GOV.UK Frontend more widely](https://github.com/alphagov/govuk-prototype-kit/pull/2317) - this makes error pages, password pages etc. more reliable. |
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 is a change log entry for a non related PR and none for this PR
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.
That's really weird ... I don't know how that got in there. I'll add one for this.
lib/utils/packageDetails.js
Outdated
@@ -219,7 +219,7 @@ async function getPluginDetailsFromRef (ref) { | |||
return await self.getInstalledPluginDetails(id) | |||
} | |||
if (source === 'fs') { | |||
return await self.getPluginDetailsFromFileSystem(id) | |||
return await self.getPluginDetailsFromFileSystem([id, ...extra].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.
I think this is a case where a comment would be helpful for anyone other than us to know what is happening here.
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'll add something to make it clearer.
beforeEach(() => () => { | ||
cy.exec(`cd ${Cypress.env('projectFolder')} && npm uninstall ${dependentPluginPackageName}`) | ||
cy.task('addToConfigJson', { allowGovukFrontendUninstall: true }) | ||
waitForApplication() |
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 found in the past that "waitForApplication()" is problematic within the beforeEach. If it's required, you need it at the start of each test. There was a change a few versions ago as to how cypress loads each test that required this.
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.
Changed, thanks.
@@ -108,6 +114,8 @@ describe('Management plugins: ', () => { | |||
deleteFile(path.join(appViews, 'step-by-step-navigation.html')) | |||
installPlugin(plugin, version2) | |||
|
|||
waitForApplication() |
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.
Not required as waitForApplication
is called within loadInstalledPluginsPage
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'll remove it.
|
||
// ------------------------ | ||
|
||
log('Fail when installing a plugin with a non existent version') | ||
cy.visit(`${managePluginsPagePath}/install?package=${encodeURIComponent(plugin)}&version=0.0.1`) | ||
cy.get('h2').contains(`Install ${pluginName}`) | ||
failAction('install') |
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.
Please see previous fail action 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.
Fair, I'll take another look at this.
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.
(as above)
@@ -76,10 +76,20 @@ describe('manage-plugins', () => { | |||
if (status === 'throws') { | |||
return Promise.reject(new Error('The server is restarting')) | |||
} | |||
return fetchResponse({ status }) |
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'm pretty sure that even though I like the use of an object of status being returned here, this will definetly break our current api and make an upgrade to this version problematic. It was the reason I didn't change it before.
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.
As long as you support the previous routes for the status call in the new version, server side, this will work,
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.
Yeah, I'll be looking at the upgrade.
@@ -149,7 +137,10 @@ | |||
</li> | |||
{% endfor %} | |||
</ul> | |||
</form> | |||
{% if showPluginLookup %} |
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.
As stated earlier, I think even with a feature flag this should be in a separate PR as it is not part of the ticket even if it is useful.
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.
The reason for it being part of this PR is that it allows us to test the functionality of looking up from Github, File System etc. Without this it's much harder to test.
It also allows us to test the plugin dependencies with local plugins.
@@ -0,0 +1,128 @@ | |||
{% from "govuk/components/radios/macro.njk" import govukRadios %} |
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 even with a feature flag this should be in a separate PR as it is not part of the ticket even if it is useful.
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.
(see other response)
@@ -219,12 +221,15 @@ function prepareWordForPackageNameDisplay (word) { | |||
} | |||
|
|||
function prepareName (name) { | |||
if (name === 'x-govuk') { |
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.
Perhaps this should be in the separate PR that supports additional plugins we haven't yet discussed as being included.
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's worth making the names clearer even if we're not including it in the "available" list because it appears once people have installed. It's a small enough change that I'd rather keep it in here but I can raise it as a separate PR if needed.
lib/utils/functionCache.js
Outdated
@@ -0,0 +1,75 @@ | |||
const { promises: fsp, existsSync } = require('fs') |
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.
Clever, but I'm a bit nervous about this being difficult for someone other than us to figure out what's happening when there is a bug that's not obvious. Perhaps some comments explaining what this is and why its used.
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.
Yeah, this was done by my IDE, I'll change it to our usual approach.
2d2e71d
to
c5119bf
Compare
Points 1 and 2 are now complete, pagination is not included. |
208861c
to
305f723
Compare
The requested changes have been made. Feel free to re-review.
7ecc397
to
bd6347c
Compare
New plugin page, this also changes the install/uninstall/update mechanism. It introduces a plugin lookup which is behind a feature flag and the option of downgrading to a specific version which is also behind a feature flag.