Skip to content
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

Ltd 5378 licence list bug #2192

Merged
merged 5 commits into from
Sep 19, 2024
Merged

Ltd 5378 licence list bug #2192

merged 5 commits into from
Sep 19, 2024

Conversation

kevincarrogan
Copy link
Contributor

@kevincarrogan kevincarrogan commented Sep 18, 2024

Aim

Fix licence list page when a good has been used multiple times on an application.

The issue was around advice objects being generated multiple times for the same good and one of the serializers falling over as a result.

The serializer for this page actually didn't need to load the advice at all so I've created a separate serializer that just sends what's required for the licence list page.

I've also separated out the Data Workspace serializer as well as this was also loading up information that it didn't need to.

LTD-5378

@kevincarrogan kevincarrogan force-pushed the LTD-5378-licence-list-bug branch 2 times, most recently from ef4e630 to 58dbf71 Compare September 19, 2024 09:34
Comment on lines +400 to +401
control_list_entries = ControlListEntrySerializer(source="good.good.control_list_entries", many=True)
assessed_control_list_entries = ControlListEntrySerializer(source="good.control_list_entries", many=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to future proof where these can be different in future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that it's wrong that we're displaying good.good.control_list_entries on the licence page now so I want to change that but I wanted to do this in a backward compatible way by adding this new field first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also adding name to the serializer, which wasn't there before, as that field is now mostly blank so I also want to change the frontend to use name.

@kevincarrogan kevincarrogan marked this pull request as ready for review September 19, 2024 11:36
@kevincarrogan kevincarrogan force-pushed the LTD-5378-licence-list-bug branch 2 times, most recently from dd765d4 to c7b8811 Compare September 19, 2024 11:38
Copy link
Contributor

@depsiatwal depsiatwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me
is there any UI changes to accompany this ?

@kevincarrogan
Copy link
Contributor Author

makes sense to me is there any UI changes to accompany this ?

Not yet. I want to deploy this first and then I can update the frontend.

@depsiatwal
Copy link
Contributor

makes sense to me is there any UI changes to accompany this ?

Not yet. I want to deploy this first and then I can update the frontend.

But we've checked it's backward compatible I mean ?

@kevincarrogan
Copy link
Contributor Author

makes sense to me is there any UI changes to accompany this ?

Not yet. I want to deploy this first and then I can update the frontend.

But we've checked it's backward compatible I mean ?

Yes. It's backwards compatible.

Copy link
Contributor

@depsiatwal depsiatwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just logged in as exporter and can confirm I can see Licences and Also caseworker side looks good also.

@kevincarrogan kevincarrogan merged commit 42124c4 into dev Sep 19, 2024
25 checks passed
Copy link
Contributor

@dejandbt dejandbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants