Merge latest Guardian changes as of 18-04-2024 #270
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...and fix byline I broke in previous commit, yeah, I dont know how to pretend I haven't
…this UGLY repetition of js bits will need to be removed
…I don’t like the hardcoding of 'label', but elementName is missing (bug?)
…yet another repetition, will remove once I know how
… to support a send to capture syndication. Still need to make changes to the syndication endpoint itself and test via postman
Added syndicated parameter to syndication metadata in the SyndicationUsageRequest. Builders in UsageGroup and UsageIdBuilder were amended to include new syndicatedBy parameter
Currently, the usage status update successfully updates dynamoDB but not ElasticSearch. In order for ElasticSearch to be updated, the correct message, containing an updated UsageNotice must be published to Thrall, via kinesis. Thrall will then process this message and run a script that will update ElasticSearch. The 'plumbing' for flow has been added, all that is left is to implement the functionality to update ElasticSearch
Further additions to the ElasticSearch file, specifically the UpdateSingleUsageMessage function. Attempted to pass usageId and usageParameters in as two tuples as 'prepareUpdateRequest' suggests that the 'params' input can have variable arguments, but was getting a syntax error. Instead choosing to just pass in usageParameters and accessing the usageId from there (if this works can remove usageId from previous functions). Some modification to the script still required I think, tested with current changes and usage status is still not updating.
It seems like the painless script is very particular with how it is written, but currently this works without an error. The script is not complete though as the parameters are being sent as Options, which painless doesn't know how to handle, so it doesn't work as expected
Params are still being sent through as Options, some difficulty in fixing this. Seems that root cause is that the play framework doesn't know how to serialise/deserialise a 'UsageNotice' as Read/Write implicits have not been defined for it. There are a few potential options to fix this. 1. Add the required implicits to UsageNotice (perhaps not desirable as would be repeated code from Usages class) 2. Refactor ThrallMessageSender to make use of Usage (or potentially MediaUsage so that we use a class it knows how to handle) 3. Copy the pattern used for the 'updateUsage' existing method, clearly can handle params properly there as they are used in that function 4. Least desirable, get the update working for the status only case which involves just two parameters, which can be unwrapped from options fairly easily, but wouldn't be applicable for generic case
Completed the work that allows for an individual usage status to be updated. The issue with Options being passed into the painless script turned out to not be the source of my problems, I was actually accessing the parameters incorrectly, with this resolved the script worked without any issues. Also cleaned up the code changes, renaming functions and parameters, so they are accurate and removing various print statements added for debugging.
Added 'lastModified' update to the update usage status painless script
Use of 'queryByUsage' meant that a Future[Status] was returned by this endpoint instead of the 'Result' type, required 'Authentication.request'. A placeholder return status was originally included so that this wouldn't block development, but once the desired functionality was achieved a permanent solution was desired. This solution was to make the endpoint explicitly asynchronous, by using 'async.auth', by default this corrected the success case and the failure case was solved by wrapping the output in a Future[]
…adata in mapping test
…resolve 'strict_dynamic_mapping_exception'
…aptureSyndication Conal/imagedam 1593 send to capture syndication
The `push-pr` workflow (added in guardian#4240) hasn't been triggering CI as it was created to do, this should fix it...
…ches trigger CI for `pr*` branches
…or_push-pr improve 'run name' for `push-pr` GHA workflow runs
…adata [feature] Hold Alt to exclude, Shift to include when clicking search links
|
looks fine to me |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
This change pulls in the latest Guardian changes into the BBC's version of the Grid, so as to bring it up to date and to pull in our changes that have been approved by the Guardian.
How should a reviewer test this change?
The changes in this PR have already been tested and approved by the Guardian. A quick sense check on the commits that this PR brings in would be good.
Tested? Documented?