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.

Sign up for GitHub

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

feat(proof): fetch and insert proof data to 'add single price' page when using 'add the price' button #584

Merged
merged 3 commits into from
Jun 5, 2024
Merged

feat(proof): fetch and insert proof data to 'add single price' page when using 'add the price' button #584

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7100aa
feat(proof): fetch and insert proof data to 'add single price' page w…
odin-h May 23, 2024
3000bb3
Forgot to change function
odin-h May 28, 2024
045528b
final cleanup
raphodn Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/views/AddPriceSingle.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,18 +412,28 @@ export default {
showUserRecentProofs() {
this.userRecentProofsDialog = true
},
handleProofSelected(proofId) {
this.addPriceSingleForm.proof_id = proofId
handleProofSelected(proof) {
Copy link
Member

Choose a reason for hiding this comment

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

the problem here is that above in mounted we only have the proof_id, so this will not work...

Copy link
Member

Choose a reason for hiding this comment

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

can be fixed by keeping proofId, and removing the date init.

Copy link
Member

Choose a reason for hiding this comment

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

and add an extra param to fetch or not the proof image ? because if coming from the UserRecentProofsDialog then it we be called even though we already have the image (line 424)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I forgot to change the function used in mounted to getProofById when copying the code over to GitHub. Did that now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what you mean by it being called twice when using the UserRecentProofsDialog though. It's only calling it once for me. Maybe I'm misunderstanding you or something is wrong with my testing environment. I couldn't get image uploading to work, uploaded pics just give me 404 error when I try to access the upload path, so I had to do a few tweaks to work around that by using remote URLs instead.

Copy link
Member

Choose a reason for hiding this comment

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

my bad there doesn't seem to be duplicate loading 👌

this.addPriceSingleForm.proof_id = proof.id
this.addPriceSingleForm.date = new Date(proof.created).toISOString().split('T')[0]
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep it empty and let the user fill it in, no ? Or some kind of warning ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When setting a recent proof or proof from URL parameter ID I definitely intended the date to be automatically set like it already does when uploading a new proof, but maybe not to the proof upload date like it does here. Preferably it should set the date to when the picture was taken instead of today's date like the current running version of the site does when selecting a proof right now. However, without adding a column for file creation time in the database, it probably have to use some potentially expensive sql queries to determine the date based on other prices uploaded to the proof already, if it even has any price entries at all. I've tried to download proof images I uploaded with exif data that has been compressed and process into mebp files, but I was not able to find any of the original exif data or dates. Only the modification date of when I uploaded the file to Open Prices. It could be that the file's exif date is stripped when serving the file through the URL (and still available internally), but it appears that maybe that data is lost in the compression process.

Copy link
Member

@raphodn raphodn Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for your detailed analysis.

the compression/conversion (to .webp) is done on the frontend side.

for the metadata you found in an already-uploaded proof image, you mention the modification date, is the date in the json returned by the server, or a date present in the metadata ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The date in the json response makes even less sense. It's before I even took the image. I'm assuming it's using a different time zone and it's actually 17:00 reflecting the date of the downloaded image:

{
  "id": 6644,
  "file_path": "0007/2rO9xlT7Nn.webp",
  "mimetype": "image/webp",
  "type": "PRICE_TAG",
  "price_count": 1,
  "owner": "odinh",
  "created": "2024-05-21T15:00:46.692372Z"
}

Here's a screenshot showing the metadata of both the downloaded image and the original:
image

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think the server is on a strange timezone or something.
maybe we could add a field "date_taken" ? though sometimes we might not have this info, if an image is uploaded without the related metadata

Copy link
Collaborator Author

@odin-h odin-h Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe changing created in the json response from a offset-naive to a offset-aware timestamp would be a good idea to do instead, so it includes the timezone offset? Disregard that, apparently the Z at the end indicates UTC.

Regarding the file metadata, could maybe make it default to the file's creation date instead, if date_taken is not available? Most files usually have a creation date even if they don't have the date_taken EXIF value. Another alternative would be to extract it from the filename like you could for the file in my example:

20240521_162617.jpg

Copy link
Member

Choose a reason for hiding this comment

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

linked issues in the backend :

this.proofImagePreview = this.getProofUrl(proof)
this.proofSelectedSuccessMessage = true
this.proofSelectedMessage = true
},
handleRecentProofSelected(selectedProof) {
this.handleProofSelected(selectedProof.id)
this.handleProofSelected(selectedProof)
this.proofImagePreview = this.getProofUrl(selectedProof)
},
getProofUrl(proof) {
return `${import.meta.env.VITE_OPEN_PRICES_APP_URL}/img/${proof.file_path}`
},
getProofById(proofId) {
this.loading = true;
api.getProofById(proofId)
.then(proof => {
this.handleProofSelected(proof);
this.loading = false;
});
},
newProof(source) {
if (source === 'gallery') {
ExifReader.load(this.proofImage[0]).then((tags) => {
Expand Down
Loading