Add json download button by sbland · Pull Request #756 · direct-framework/direct-webapp

sbland · 2026-07-04T21:03:40Z

Description

Implements a json download link in addition to the csv download link added previously.

Also improved the share button docs

Fixes #742
Fixes: #770

Type of change

Documentation (non-breaking change that adds or improves the documentation)
New feature (non-breaking change which adds functionality)
Optimization (non-breaking, back-end change that speeds up the code)
Technical work (non-breaking, change which is work as part of a new feature)
Bug fix (non-breaking change which fixes an issue)
Breaking change (whatever its nature)

Key checklist

All tests pass (eg. python -m pytest)
The documentation builds and looks OK (eg. mkdocs serve)
Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

Code is commented, particularly in hard-to-understand areas
Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

codecov · 2026-07-04T21:05:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

sbland · 2026-09-20T19:57:32Z

@AdrianDAlessandro I've rewritten this to us django data endpoint as we discussed instead of using javascript.

This approach doesn't work so well with the shared page so I've disabled the download buttons on that page and added an issue to follow up here: #788

I think this should be ready to merge now but let me know what you think.

AdrianDAlessandro

This is close. I am happy with the architecture approach. Just a few small details in the views themselves

AdrianDAlessandro · 2026-10-06T11:45:41Z

(Not for this PR) I wonder if these snippets of JS scripts should be in a separate folder, or sub folder? They seem like they should be considered a separate category from the HTML snippets that are intended to be in this folder. Perhaps in main/templates/main/snippets/scripts or main/templates/main/scripts?

AdrianDAlessandro · 2026-10-06T12:56:59Z

+class DownloadUserSkillDataJSONView(View):
+    """A view that returns the user skill data as a JSON file."""
+
+    def get(self, request: HttpRequest) -> JsonResponse:


The business logic (of building the dictionary) in this method is the same as for the CSV get method. The difference between them is in how user_skills_data is used later. Can that part of the code be abstracted into a standalone function?

I've updated the views to use a mixin and base view. See code comments.

AdrianDAlessandro · 2026-10-06T13:00:52Z

+
+
+@pytest.fixture
+def client_logged_in(client, user):


Didn't realise we didn't have one of these!

Let me know if you want me to pull out to another PR or just slide in with this one.

No, it's fine to be in this PR. Adding these kinds of thing as and when they're needed makes the most sense to me

sbland · 2026-10-14T11:34:33Z

+    from ..models import User as UserType
+
+
+class _AuthenticatedHttpRequest(HttpRequest):


@AdrianDAlessandro I added "_" at the start of these so they are not imported into views/init.py which was causing type errors.

These shouldn't be necessary at all (with importing strategy I mentioned elsewhere). But out of curiosity, what were the type errors?

The type error was something to do with request.user cannot be assigned to User from models

sbland · 2026-10-14T11:35:06Z

+
+from ..models import User, UserSkill
+
+logger = logging.getLogger(__name__)


@AdrianDAlessandro Some of this is duplicated in account_views.py

Duplication shouldn't be required if we just import the TermsAcceptedMixin

sbland · 2026-10-14T11:37:27Z

+        return super().dispatch(request, *args, **kwargs)  # type: ignore
+
+
+class AuthenticatedDataView(_TermsAcceptedMixin, View):


@AdrianDAlessandro previously these endpoints were not requiring authentication so I've corrected this with a base class

I don't think this base class adds anything, just use the TermsAcceptedMixin directly in the view.

sbland · 2026-10-14T11:38:08Z

+    request: _AuthenticatedHttpRequest
+
+
+class DownloadUserSkillDataMixin:


@AdrianDAlessandro I've used a mixin for this but we could have just as easily used a standalone function. Let me know what you think.

I think a standalone function is more appropriate here. Mixins are for adding web-specific functionality, but this is just a database accessor

AdrianDAlessandro · 2026-10-14T13:07:43Z

About to start reviewing this but worth noting there are some merge conflicts to resolve. So get to those ASAP

Added the csv and json download data views that can be accessed from: - http://localhost:8000/accounts/download-skills-data/csv - http://localhost:8000/accounts/download-skills-data/json Also included an optional parameter to the json download to export anonymous json

Also made this togglable as they are no longer valid for the shared skills data view

Changed from category - competency_domain subcategory - competency

sbland · 2026-10-14T13:15:29Z

@ad

About to start reviewing this but worth noting there are some merge conflicts to resolve. So get to those ASAP

Rebased to resolve conflicts

AdrianDAlessandro

I've made a few suggestions, mainly to just pare back the extra boilerplate added. We really just need that _format_user_skill_data_for_download method as a function to be used in the views as they were structured before. If you want to add the authentication required element, then just import the existing mixin

AdrianDAlessandro · 2026-10-14T13:30:44Z

I don't think we need this file. You should be able to import the AuthenticatedHttpRequest and TermsAcceptedMixin straight into data_views.py and make these views require authentication by using that directly as a mixin.

I partly agree except for two points:

Importing between views doesn't seem right so perhaps we need a separate shared utils file?

We have a separate file for account views so it seemed to make sense to have a separate one for account data views too.

I wasn't sure about having the dataview endpoints redirect to the login page which is what the TermsAcceptedMixin does. This would only actually happen if someone tried to go to the download link directly. I can revert to using the existing mixins.

AdrianDAlessandro · 2026-10-14T13:31:20Z

+
+from ..models import User, UserSkill
+
+logger = logging.getLogger(__name__)


Duplication shouldn't be required if we just import the TermsAcceptedMixin

AdrianDAlessandro · 2026-10-14T13:32:30Z

+    from ..models import User as UserType
+
+
+class _AuthenticatedHttpRequest(HttpRequest):


These shouldn't be necessary at all (with importing strategy I mentioned elsewhere). But out of curiosity, what were the type errors?

AdrianDAlessandro · 2026-10-14T13:35:27Z

+        return super().dispatch(request, *args, **kwargs)  # type: ignore
+
+
+class AuthenticatedDataView(_TermsAcceptedMixin, View):


I don't think this base class adds anything, just use the TermsAcceptedMixin directly in the view.

AdrianDAlessandro · 2026-10-14T13:38:36Z

+    request: _AuthenticatedHttpRequest
+
+
+class DownloadUserSkillDataMixin:


I think a standalone function is more appropriate here. Mixins are for adding web-specific functionality, but this is just a database accessor

AdrianDAlessandro · 2026-10-14T13:42:31Z

+from django.urls import reverse
+
+
+class ViewRequiresAuthenticationMixin:


Should be able to use the LoginRequiredMixin here instead

As in the account_data_views.py comments should we extract this class to a separate util file?

… views

sbland · 2026-10-15T06:56:26Z

@AdrianDAlessandro I've used the mixins from the account views instead of recreating for the data account views. I've kept the separate file for now but I think some thought needs to go into how these files are arranged. Currenlty we have a separate account_view.py file but this and the page_views.py are both tested in the test_main_views.py file.

…ount data views

for more information, see https://pre-commit.ci

AdrianDAlessandro

Great! This is looking good. Just some failing listing checks in pre-commit to fix up and then feel free to merge!

Also, I agree about the overall structure needing some thought / improvement. And definitely we need to improve the consistency between the main folder and the tests folder.

This comment was marked as resolved.

Sign in to view

sbland self-assigned this Aug 13, 2026

sbland added the pr-change-required label Aug 27, 2026

sbland force-pushed the 742-profile-skills-exportimport-buttons/create-json-download-button branch from 47bd9f7 to 21d0b85 Compare September 20, 2026 19:48

sbland requested a review from AdrianDAlessandro September 20, 2026 19:53

sbland force-pushed the 742-profile-skills-exportimport-buttons/create-json-download-button branch from 24215c7 to ff20ac5 Compare September 20, 2026 20:27

sbland assigned AdrianDAlessandro and unassigned sbland Oct 1, 2026

sbland added this to Web App Development Oct 1, 2026

AdrianDAlessandro reviewed Oct 6, 2026

View reviewed changes

AdrianDAlessandro removed the pr-change-required label Oct 9, 2026

sbland commented Oct 14, 2026

View reviewed changes

sbland force-pushed the 742-profile-skills-exportimport-buttons/create-json-download-button branch from 340ffd5 to 912bc19 Compare October 14, 2026 11:37

sbland commented Oct 14, 2026

View reviewed changes

sbland added 8 commits October 14, 2026 14:12

Add json download button

d3ef35c

Update skill share inline docs

6c3f13d

Split export skills js

643bd14

Add client logged in fixture

6e43ec3

Update download json and csv links in skill share dropdown

29b6b95

Also made this togglable as they are no longer valid for the shared skills data view

Fix exported data field names

0c796d3

Changed from category - competency_domain subcategory - competency

Moved account data views to separate files and implemented mixins

cc3824c

sbland force-pushed the 742-profile-skills-exportimport-buttons/create-json-download-button branch from 912bc19 to cc3824c Compare October 14, 2026 13:13

AdrianDAlessandro requested changes Oct 14, 2026

View reviewed changes

Used Mixins from account views instead of recreating for data account…

ac3b908

… views

sbland force-pushed the 742-profile-skills-exportimport-buttons/create-json-download-button branch from a99b2da to ac3b908 Compare October 15, 2026 06:51

Use function instead of mixin class for format user skill data in acc…

ff557fb

…ount data views

sbland force-pushed the 742-profile-skills-exportimport-buttons/create-json-download-button branch from 3441c43 to ff557fb Compare October 15, 2026 06:58

[pre-commit.ci] auto fixes from pre-commit.com hooks

6e89962

for more information, see https://pre-commit.ci

AdrianDAlessandro approved these changes Oct 15, 2026

View reviewed changes

sbland added 2 commits October 15, 2026 11:01

Minor amends to download skill profile button ids and selectors

531c29a

Minor amend to download file name

ff58c7d

sbland merged commit 0da7e7c into main Oct 15, 2026
7 checks passed

sbland deleted the 742-profile-skills-exportimport-buttons/create-json-download-button branch October 15, 2026 11:04

github-project-automation Bot moved this from 👀 In review to ✅ Done in Web App Development Oct 15, 2026

		from ..models import User as UserType


		class _AuthenticatedHttpRequest(HttpRequest):


		from ..models import User, UserSkill

		logger = logging.getLogger(__name__)

		return super().dispatch(request, args, *kwargs) # type: ignore


		class AuthenticatedDataView(_TermsAcceptedMixin, View):

		request: _AuthenticatedHttpRequest


		class DownloadUserSkillDataMixin:

		from django.urls import reverse


		class ViewRequiresAuthenticationMixin:

Conversation

sbland commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov Bot commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as resolved.

Uh oh!

sbland commented Sep 20, 2026

Uh oh!

AdrianDAlessandro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdrianDAlessandro commented Oct 14, 2026

Uh oh!

sbland commented Oct 14, 2026

Uh oh!

AdrianDAlessandro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbland commented Oct 15, 2026

Uh oh!

AdrianDAlessandro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbland commented Jul 4, 2026 •

edited

Loading

codecov Bot commented Jul 4, 2026 •

edited

Loading