Repository navigation
CDD-3349 - #3366
CDD-3349#3366manubinary wants to merge 89 commits into
Conversation
linitng issue
test issue fix
unit test fiixes
mock file updated
required changes
jrdh
left a comment
jrdh
left a comment
There was a problem hiding this comment.
Apologies this took so long! I think the approach needs to change slightly here and I think doing that will make some of this a bit easier to manage because you can then rely on always having a unique topic / sub-theme / theme combination. The main comment on the logic is here.
| page_classification=data["page_classification"] | ||
| page_classification=data["page_classification"], | ||
| theme=data["theme"], | ||
| sub_theme=data["sub_theme"], |
There was a problem hiding this comment.
Since #3362 was merged, the fields are called page_theme and page_sub_theme (and page_topic just to be complete) now. The new fields are defined in the NonPublicCapablePage abstract base class and the only reason the old ones without the page_ prefix are still there is because they were required for rollback in case of an issue with the migration to the new field names. I should have marked them up a bit more obviously with a comment to say that they shoudn't be used - apologies! They will be removed once I rebase this PR.
Note that this applies to all the json files as well - though see my comments saying that I don't think they are necessary there.
| topic=selected_topic, | ||
| theme=page.theme_name or "", | ||
| sub_theme=page.sub_theme_name or "", | ||
| ) |
There was a problem hiding this comment.
Given None is allowed by InternalAPIClient.hit_geographies_list_endpoint for theme and sub_theme, why or ""?
| "selected_topics": [] | ||
| "selected_topics": [], | ||
| "theme": "immunisation", | ||
| "sub_theme": "childhood-vaccines" |
There was a problem hiding this comment.
On pages that is_public=true the theme, sub_theme, and topic (which you'll notice isn't in this json) are blanked during the clean part of the save process so these are unecessarily.
| if not self.sub_theme: | ||
| return None | ||
| return self.sub_theme | ||
|
|
There was a problem hiding this comment.
I don't think this is the right way to go about this for a couple of reasons - at least not yet.
At the moment the access control fields (the ones defined in the NonPublicCapablePage - is_public, page_classification, page_theme, page_sub_theme, page_topic) are purely for access control, they aren't used as part of the page content management. As a specific example, we implemented the addition of the theme and sub_theme to each chart request, not by using the page's page_theme and page_sub_theme, but by adding them to the form for each chart. This was to keep that separation and because page_theme and page_sub_theme aren't available on all page types that can have charts (e.g. anything non-topic like the landing page). I think this change should follow that same pattern until such a time as we change that and therefore using the page_theme and page_sub_theme (see this comment about field names)) isn't the right way to get this data. In the future we may change that approach so it's not necessarily the right idea but it's not the right solution right now unfortunately because of the way we've set this up.
The second issue (and this relates to the above issue as well) is that as it stands, a topic page doesn't have to be about a single topic. I don't know the history of this or whether it's even in use in prod but the evidence is that there's no topic selected for a topic page which is then used to filter the available metrics and, crucially for this PR, the use of selected_topics used by the area selector. This selected_topics API field is a list of topics that appear on the page. I think we should implement this in the same way to allow multiple themes and sub-themes to appear on a page and decouple the logic from the access control fields. The simplest way to do this, I think, is to create selected_topics @property methods like selected_topics and selected_metrics which use the body of the page to extract the themes and sub-themes from the charts themselves and return lists of them. The only issue you may have with that is that you need to match the topics, themes, and sub-themes up on the frontend and using index order is a bit flakey and indirect, so it may be better to changed the selected_topics API field to return a list of tuples instead like:
@property
def selected_a_good_name(self) -> set(tuple[str, str, str]):
...
which ultimately returns something like:
# a set of tuples with each containing a theme, sub-theme, and topic
{
("infectious_disease", "respiratory", "COVID-19"),
("infectious_disease", "respiratory", "Influenza"),
("immunisation", "childhood-vaccines", "MMR1"),
}
There was a problem hiding this comment.
As discussed, the theme and sub-theme values are not available for cover pages because their charts are generated dynamically based on the selected filters, unlike topic pages. Therefore, we cannot extract these values from the chart configurations or implement the same approach for cover pages.
| class GeographiesForTopicSerializer(serializers.Serializer): | ||
| topic = serializers.CharField() | ||
| theme = serializers.CharField(required=False, default="") | ||
| sub_theme = serializers.CharField(required=False, default="") |
There was a problem hiding this comment.
Related to this other comment, I think if we make the change to use the actual themes and sub-themes on a page by extracting them from the charts, we can make the required fields. This does have an added side benefit (which was also realised in the PR adding theme and sub-theme to charts) that we close a nasty edge case where the same topic name can be under different themes / sub-themes. Technically a topic is only unique if it comes with the theme and sub-theme so always providing it even in public contexts is a good thing.
| request=request, | ||
| data=data, | ||
| theme=serializer.validated_data.get("theme", ""), | ||
| sub_theme=serializer.validated_data.get("sub_theme", ""), |
There was a problem hiding this comment.
Cascading on from the other comments about always providing theme and sub-theme, this would then allow these to be required and not have a default value.
| queryset = queryset.filter( | ||
| metric__topic__sub_theme__name=sub_theme, | ||
| metric__topic__sub_theme__theme__name=theme, | ||
| ) |
There was a problem hiding this comment.
I think these should be required because of the lack of topic name uniqueness mentioned in previous comments.
| topic_id = str(topic_id) if topic_id else "" | ||
|
|
||
| # Step 1: only permission_sets whose theme/sub_theme (and topic, if relevant) | ||
| # match the page we're rendering are "in scope" for this check at all. |
There was a problem hiding this comment.
bit nitpicky but also clarity is good - not sure how the page relates to the check being done here?
required changes
required changes
…ithub.com/UKHSA-Internal/data-dashboard-api into origin/CDD-3349-geography-API-permissions
required changes
test failure fixes
for public pages theme and sub_theme should be empty values
|
CDD-3349
Description
This PR includes the following:
Changes
themeandsub_themevalues.themeandsub_themevalues for permission checks.Fixes #CDD-<ISSUE_ID>
Type of change
Please select the options that are relevant.
Checklist: