2319 add sidebar to admin decision data page #630
Conversation
richpjames
force-pushed
the
2319-add-sidebar-to-admin-decision-data-page
branch
8 times, most recently
from
e8a8225
to
d6667a6
Compare
| @@ -433,11 +433,30 @@ describe('DecisionsController', () => { | |||
| mockTables, | |||
| ); | |||
|
|
|||
| Object.defineProperties(DecisionDatasetPresenter.prototype, { | |||
| changedBy: { | |||
There was a problem hiding this comment.
This is a workaround for this bug: jestjs/jest#9675
TLDR: Jest's spyOn doesn't work for instance getters
There was a problem hiding this comment.
This means we can't assert that the presenter has been called with the expected values as we can't spy on the getters but this functionality is tested, at a higher level, in the e2e tests
| @@ -63,7 +63,7 @@ describe('formatStatus', () => { | |||
| it('returns a yellow tag', () => { | |||
| const i18nService = createMockI18nService(); | |||
|
|
|||
| const result = formatStatus(OrganisationVersionStatus.Draft, i18nService); | |||
| const result = formatStatus(DecisionDatasetStatus.Draft, i18nService); | |||
There was a problem hiding this comment.
I'm a bit unclear on the reason behind this change. The commit message suggests this is adding a new status, but weren't we already showing the status in the decision data table? I think we can probably remove this commit.
If you're keen on having a test for this status, it's probably an idea to either do it in its own test, or test a couple of variables in this test with different Draft statuses, but I don't think it's necessary here.
There was a problem hiding this comment.
Yeah looking back I think this test is redundant and removing this change is the best option.
| }); | ||
| }); | ||
|
|
||
| it("if the dataset doesn't have a user it returns their name and email address", () => { |
There was a problem hiding this comment.
Should this be returns null?
| ); | ||
| }); | ||
|
|
||
| it('I can publish the data by clicking the publish button', () => { |
There was a problem hiding this comment.
This is covered by the publish.spec now, so I think we can remove it from here.
views/admin/decisions/show.njk
Outdated
| @@ -26,7 +51,7 @@ | |||
| }} | |||
| </li> | |||
| {% endif %} | |||
| {% if 'submitDecisionData' in permissions and not (datasetStatus === 'submitted') %} | |||
| {% if 'submitDecisionData' in permissions and not(datasetStatus === 'submitted') %} | |||
There was a problem hiding this comment.
Was this not working as expected before?
There was a problem hiding this comment.
This was formatted automatically on save by Prettier. I opted to go with it as it doesn't seem to make a difference semantically and it's easier to not be fighting Prettier but I am happy to back it out if you'd prefer
There was a problem hiding this comment.
I'm not sure how reliable Prettier is with njk files (we've been struggling to find a reliable config), but it might be better to commit things like this in their own commit (e.g. Format file with prettier) so it doesn't introduce overhead in another commit. I don't mind this going in now though.
There was a problem hiding this comment.
Good point - in future I will add formatting in a seperate commit.
I am going to re-add the space so it's consistent with the rest of the codebase
richpjames
force-pushed
the
2319-add-sidebar-to-admin-decision-data-page
branch
3 times, most recently
from
c1ffc13
to
c250ec0
Compare
we will need users to enable showing who edited the data last
containing status, last modified date, changed by, link to currently published version and edit button
richpjames
force-pushed
the
2319-add-sidebar-to-admin-decision-data-page
branch
from
c250ec0
to
f0a5977
Compare
Changes in this PR
This PR adds a sidebar to the admin decision data page which contains:
A button to download the dataset will be added in a subsequent PR
Screenshots of UI changes
Before
After