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

Fix for #5808: Update the cached Place Items on the successful association of Wiki Item #5864

Merged
merged 9 commits into from
Oct 19, 2024

Conversation

ChristoJobyAntony
Copy link
Contributor

@ChristoJobyAntony ChristoJobyAntony commented Oct 17, 2024

When a user uploads a valid depiction of a Wiki Item which currently has no image (represented by a red pin), the pin should turn green on the successful upload and depiction of the image. However, due to caching of Nearby places, the pin colour is not updated after user's image upload.

Fixes #5808

Changes

  1. A new method was added to the PlacesRepository to delete a given Place entity.
  2. On the successful claiming of a WikiItemEdit, the corresponding Wiki Item is cleared from the cache
  3. On the successful claiming of a WikiItemEdit, the corresponding Wiki Item updated in the local cache.
  4. The updated link is created using the format: HOME_URL + uploadResult.createCanonicalFileName()
  5. The field name of this Place object is also updated from the WikiItem. This is because there was a scenario where a Place fetched would have an empty name. Therefore, the NearbyParentFragment tries to update Places without a valid name from the web, leading to a race condition. This situation was observed through logs.
    image

Tested ProdDebug on Pixel 7 with API level 34.
https://github.com/user-attachments/assets/6328b8ca-8342-453b-815a-30f0c10d36f2

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Is it still a draft?

Fantastic code, and screencast looks great!

@ChristoJobyAntony ChristoJobyAntony changed the title Fix for 5808: Clear the cached Place Items on the successful association of Wiki Item Fix for #5808: Clear the cached Place Items on the successful association of Wiki Item Oct 18, 2024
@ChristoJobyAntony
Copy link
Contributor Author

Is it still a draft?

@nicolas-raoul, I had left the PR as draft to get your feedback on the approach I had taken. If you think this is approach is acceptable, I am happy to set this PR to ready to review.

@ChristoJobyAntony ChristoJobyAntony marked this pull request as ready for review October 18, 2024 01:13
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Code looks great!
I am testing on my device, now going outside to actually get pictures of places and try it out in the real world. :-)

@nicolas-raoul
Copy link
Member

Somehow the pin stayed red in my test (screencast below)... any idea what might be happening?

Maybe a server-side cache issue? Analyzing server response to our query_for_item.rq request would tell. In that case deleting the cache again every 10 seconds for 1 minute may be a workaround.

screen-20241018-115915.mp4

@ChristoJobyAntony
Copy link
Contributor Author

ChristoJobyAntony commented Oct 18, 2024

@nicolas-raoul

Somehow the pin stayed red in my test (screencast below)... any idea what might be happening?

Oh, that is very interesting. The only reason I can possibly attribute to is that I had added a breakpoint at the step to check whether the cache was being cleared. This could have possibly added enough delay for the server side response to be updated.

Maybe a server-side cache issue? Analyzing server response to our query_for_item.rq request would tell. In that case deleting the cache again every 10 seconds for 1 minute may be a workaround.

Alternatively, do you think it would be appropriate to instead directly write the updated image link to the cache, this will only be done if a revisionID is returned. It would reduce the requests required to be sent out.

@nicolas-raoul
Copy link
Member

Alternatively, do you think it would be appropriate to instead directly write [...] to the cache

Usually it is more elegant (even if a bit wasteful) but only clear the cache and then let the normal code load the new value from the server. But in this case I agree it is a good way to avoid server-side cache issues.

Looking forward to the updated pull request, thanks a lot for the great idea!

@ChristoJobyAntony
Copy link
Contributor Author

ChristoJobyAntony commented Oct 18, 2024

@nicolas-raoul Thank you for the feedback. I agree that updating the URL would be the most reliable solution at this point, without adding additional overhead. However, when inspecting the code, I realised that after a successful upload, the UploadResult object doesn't contain the uploaded image's URI. Only the uploaded filename. Do you think it is fine if I reconstruct the URI based on the pattern "HOME_URL + uploadResult.createCanonicalFileName()" ? Is there a more pragmatic approach to getting the Uploaded URL ?

@ChristoJobyAntony ChristoJobyAntony changed the title Fix for #5808: Clear the cached Place Items on the successful association of Wiki Item Fix for #5808: Update the cached Place Items on the successful association of Wiki Item Oct 18, 2024
@ChristoJobyAntony
Copy link
Contributor Author

@nicolas-raoul I have updated the PR description, and also updated the code to reflect our conversation. Would you be able to review this PR ?

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Works great, thanks a lot!

screen-20241019-220707.mp4

@nicolas-raoul nicolas-raoul merged commit 63f1ed8 into commons-app:main Oct 19, 2024
1 check passed
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.

Nearby: invalidate cache after uploading from Nearby
2 participants