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

Show torrent fields: creation_date, created_by, encoding #297

Conversation

mario-nt
Copy link
Contributor

Resolves #278.

@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch 2 times, most recently from f5efbd9 to fe4e157 Compare October 25, 2023 12:56
@josecelano josecelano added the Needs Rebase Base Branch has Incompatibilities label Oct 27, 2023
@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch from fe4e157 to 81fa3bb Compare November 7, 2023 16:21
@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch from 81fa3bb to d38abfc Compare November 15, 2023 23:57
@mario-nt mario-nt marked this pull request as ready for review November 16, 2023 00:04
@mario-nt
Copy link
Contributor Author

@josecelano Rebased and CI OK

}
});

// Takes the date in milliseconds/Epoch Time and converts it to human readable format
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt,

  • I would move the comment to the function. If you think the name is not enough, you could change the name to epochTimeToHumanReadableUTC or timestampToHumanReadableUTC
  • Timestamp is in seconds (props.torrent.creation_date), not milliseconds. The Date constructor accepts milliseconds (new Date(date * 1000);). We are passing seconds to the function.

Comment on lines 1 to 21
export function epochToUtc (date: number) {
const convertedDate = new Date(date * 1000);
return convertedDate.toDateString();
}
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt since we are using TypeScript maybe we can introduce a new Timestamp type to make it explicit what the function needs.

Copy link
Member

Choose a reason for hiding this comment

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

I was using snake_case for JS functions and I guess also for filenames because they all start with a lowercase:

  • sanitizer.ts
  • slug.ts

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add a unit test for the function but it's a pain to add unit tests with Nuxt.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt since we are using TypeScript maybe we can introduce a new Timestamp type to make it explicit what the function needs.

Maybe something like:

type Timestamp = number;

export function epochToUtc (timestamp: Timestamp) {
  new Date(timestamp.toMilliseconds()).toDateString();
}

But you can even use a field and check that the number is an integer.

@josecelano
Copy link
Member

This is how the detail page looks with the new fields:

image

@mario-nt mario-nt marked this pull request as draft November 20, 2023 12:51
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 22, 2023
If the torrent creation date field is present in the torrent file, it is now shown as a UTC date
in a human readable format.

If there is no creation date, a no creation date provided message is displayed.

And if there is an error parsing the creation date or if it is a no valid date, an invalid date
text is shown.
@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch 7 times, most recently from 003c80d to f4b5ca6 Compare November 27, 2023 10:49
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 27, 2023
Error checks added to the function that converts the torrent creation date from seconds to
UTC human readable format.

Refactor to the function logic's code has been applied.
@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch from f4b5ca6 to ad0c609 Compare November 27, 2023 15:20
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 27, 2023
If the torrent creation date field is present in the torrent file, it is now shown as a UTC date
in a human readable format.

If there is no creation date, a no creation date provided message is displayed.

And if there is an error parsing the creation date or if it is a no valid date, an invalid date
text is shown.
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 27, 2023
Error checks added to the function that converts the torrent creation date from seconds to
UTC human readable format.

Refactor to the function logic's code has been applied.
@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch from ad0c609 to 89f3f0c Compare November 27, 2023 15:39
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 29, 2023
Date converter helper function now throws errors if the timestamp passed as argument is not an integer
and if the Date() constructor returns an invalid date.
@mario-nt mario-nt assigned mario-nt and unassigned mario-nt Nov 29, 2023
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 29, 2023
If the torrent creation date field is present in the torrent file, it is now shown as a UTC date
in a human readable format.

If there is no creation date, a no creation date provided message is displayed.

And if there is an error parsing the creation date or if it is a no valid date, an invalid date
text is shown.
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 29, 2023
Error checks added to the function that converts the torrent creation date from seconds to
UTC human readable format.

Refactor to the function logic's code has been applied.
mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Nov 29, 2023
Date converter helper function now throws errors if the timestamp passed as argument is not an integer
and if the Date() constructor returns an invalid date.
@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch from 1443c0e to f6ff259 Compare November 29, 2023 19:41
@mario-nt mario-nt marked this pull request as ready for review November 29, 2023 19:48
@mario-nt
Copy link
Contributor Author

@josecelano Ready for review.

mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Dec 3, 2023
@mario-nt
Copy link
Contributor Author

mario-nt commented Dec 3, 2023

@josecelano Ready for review.

<template v-if="!collapsed">
<div class="flex flex-col w-full h-full p-6 grow bg-base-100 rounded-2xl">
<template v-if="torrent.creation_date">
{{ unixTimeToHumanReadableUTC(torrent.creation_date) }}
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt maybe inside the components we could use a wrapper helper with the name formatDateFromTimestamp.

class InvalidDateError extends Error {}
class WrongTimestamp extends Error {}

// Takes the date in seconds from Epoch time and converts it to human readable format.
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt I think we should use a format for comments like https://tsdoc.org/ but we can enforce that after installing MegaLinter.

try {
milliseconds = creationDate * 1000;
} catch (error) {
return new WrongTimestamp(`Could not convert ${creationDate} to milliseconds`);
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt I think this line is never reached. If I'm not wrong miliseconds would be Infinity but ti will not throw an exception.

// Online Javascript Editor for free
// Write, Edit and Run your Javascript code using JS Online Compiler

let seconds = Number.MAX_VALUE;

let miliseconds = seconds * 1000;
console.log("test1: ", miliseconds); // Infinity

try {
  milliseconds = seconds * 1000;
} catch (error) {
  console.log("error", error);
}
console.log("test2: ", miliseconds); // Infinity

} catch (error) {
return new InvalidDateError(`Could not create a new date from ${milliseconds}`);
}
return !isNaN(convertedDate.valueOf()) ? convertedDate.toDateString() : new InvalidDateError(`Could not create a valid date from ${milliseconds}`);
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt In general I prefer positive conditional:

  return isNaN(convertedDate.valueOf())
    ? new InvalidDateError(
        `Could not create a valid date from ${milliseconds}`
      )
    : convertedDate.toDateString();

See https://softwareengineering.stackexchange.com/a/350474/430081

Comment on lines 16 to 20
try {
convertedDate = new Date(milliseconds);
} catch (error) {
return new InvalidDateError(`Could not create a new date from ${milliseconds}`);
}
Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt I think the <Date function does not thorw any exception:

let seconds = Number.MAX_VALUE;

let miliseconds = seconds * 1000;
console.log("test1: ", miliseconds); // Infinity

try {
  milliseconds = seconds * 1000;
} catch (error) {
  console.log("overflow error", error);
}
console.log("test2: ", miliseconds); // Infinity

try {
  convertedDate = new Date(seconds * 1000);
} catch (error) {
  console.log("Date error", error);
}
console.log("test3: ", convertedDate); // Infinity

Copy link
Member

Choose a reason for hiding this comment

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

Some tests I did:

let seconds = Number.MAX_VALUE;

let miliseconds = seconds * 1000;
console.log("test1: ", miliseconds); // Infinity

try {
  milliseconds = seconds * 1000;
} catch (error) {
  console.log("overflow error", error);
}
console.log("test2: ", miliseconds); // Infinity

try {
  convertedDate = new Date(seconds * 1000);
} catch (error) {
  console.log("Date error", error);
}
console.log("test3: ", convertedDate); // Invalid Date

convertedDate = new Date(1701688171 * 1000);
console.log("test4: ", convertedDate);
console.log("test5: ", convertedDate.toISOString());
console.log("test6: ", convertedDate.toUTCString());

Copy link
Member

Choose a reason for hiding this comment

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

Hi @mario-nt I've just created another example.

Why do you want to differentiate between an invalid timestamp in the database and a timestamp greater than the maximum Date? Now we are showing the value causing the error so we can find out later what happened.

mario-nt added a commit to mario-nt/torrust-index-gui that referenced this pull request Dec 20, 2023
@mario-nt
Copy link
Contributor Author

@josecelano Changes applied.

@josecelano
Copy link
Member

@josecelano Changes applied.

Hi @mario-nt it looks good but it needs to be rebased.

MMelchor and others added 6 commits December 23, 2023 01:42
…ding

    Torrent details page now displays the creation date, created by and encoding fields.
If the torrent creation date field is present in the torrent file, it is now shown as a UTC date
in a human readable format.

If there is no creation date, a no creation date provided message is displayed.

And if there is an error parsing the creation date or if it is a no valid date, an invalid date
text is shown.
Error checks added to the function that converts the torrent creation date from seconds to
UTC human readable format.

Refactor to the function logic's code has been applied.
Date converter helper function now throws errors if the timestamp passed as argument is not an integer
and if the Date() constructor returns an invalid date.
@mario-nt mario-nt force-pushed the #278-show-torrent-fields-creation_date-created-by-encoding branch from 85a927c to 4281526 Compare December 23, 2023 01:08
@mario-nt
Copy link
Contributor Author

@josecelano Rebased.

@josecelano
Copy link
Member

ACK 4281526

@josecelano josecelano merged commit b61905b into torrust:develop Dec 26, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase Base Branch has Incompatibilities
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Show torrent fields: creation_date, created_by, encoding
2 participants