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(client): avatar url #2791

Merged
merged 5 commits into from Sep 14, 2023
Merged

fix(client): avatar url #2791

merged 5 commits into from Sep 14, 2023

Conversation

andre-code
Copy link
Contributor

@andre-code andre-code commented Sep 11, 2023

PR to resolve the dataset avatar URL issue. GitHub URL is included only if the content_url is a GitHub URL.

Testing

Every dataset in this project should display an image. https://renku-ci-ui-2791.dev.renku.ch/projects/lorenzo.cavazzi.tech/test-dataset-avatars/datasets

Fix #2765

/deploy #persist

@andre-code andre-code force-pushed the 2765-fix-dataset-avatar-url branch 2 times, most recently from c89ab0e to 04af804 Compare September 11, 2023 15:33
@andre-code andre-code temporarily deployed to renku-ci-ui-2791 September 11, 2023 15:35 — with GitHub Actions Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-2791.dev.renku.ch

@andre-code andre-code temporarily deployed to renku-ci-ui-2791 September 12, 2023 07:08 — with GitHub Actions Inactive
@andre-code andre-code marked this pull request as ready for review September 12, 2023 07:08
@andre-code andre-code requested a review from a team as a code owner September 12, 2023 07:08
Copy link
Member

@leafty leafty Sep 12, 2023

Choose a reason for hiding this comment

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

CODING_GUIDELINES.md

This file should probably be renamed to datasets.utils.ts or similar.

@andre-code andre-code temporarily deployed to renku-ci-ui-2791 September 12, 2023 07:30 — with GitHub Actions Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2791 September 12, 2023 07:42 — with GitHub Actions Inactive
interface DatasetImages {
content_url: string;
}
export interface IDataset {
Copy link
Member

Choose a reason for hiding this comment

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

If possible, interfaces should not use the I prefix:

Suggested change
export interface IDataset {
export interface Dataset {

* @param {Dataset} dataset
*/
export function addMarqueeImageToDataset(gitUrl: string, dataset: IDataset) {
const urlRoot = cleanGitUrl(gitUrl) + "/-/raw/master";
Copy link
Member

Choose a reason for hiding this comment

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

The logic is incorrect here, we should not assume that the default branch for a git repository is called master. The project data includes the name of the default branch, e.g.

const defaultBranch = useSelector<RootStateOrAny, string>(
  (state) => state.stateModel.project.metadata.defaultBranch
);
Suggested change
const urlRoot = cleanGitUrl(gitUrl) + "/-/raw/master";
const urlRoot = `${cleanGitUrl(gitUrl)}/-/raw/${defaultBranch}`;

Comment on lines 39 to 40
dataset.mediaContent = mediaUrl;
return dataset;
Copy link
Member

Choose a reason for hiding this comment

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

[nitpick] It usually is less bug-prone to avoid mutating arguments in function. Code here could return an updated object, e.g.

Suggested change
dataset.mediaContent = mediaUrl;
return dataset;
return {
...dataset,
mediaContent: mediaUrl,
};

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 understand the suggestion to avoid mutating arguments in functions. In this specific context, we intend to mutate the object when fetching datasets in order to add the mediaContent property. While we could refactor this code chain to return a new object, it's not within the scope of this PR. We may consider making such a change when transitioning to the use of Redux Toolkit

export function addMarqueeImageToDataset(gitUrl: string, dataset: IDataset) {
const urlRoot = cleanGitUrl(gitUrl) + "/-/raw/master";
let mediaUrl = dataset?.images?.[0]?.content_url ?? null;
if (mediaUrl !== null && mediaUrl.startsWith(".renku/dataset_images/"))
Copy link
Member

Choose a reason for hiding this comment

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

I find this logic non-intuitive here. Why do we not consider any relative path inside the repository? Non-relative locations would start with a URL scheme (<scheme>://). Also, we should probably validate external URLs and only allow those starting with https://.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion is to use the URL class:

const mediaUrl = new URL("https://example.com/image.jpg", "https://gitlab.renkulab.io/namespace/name/-/raw/main/");
mediaUrl.toString() // 'https://example.com/image.jpg'
const mediaUrl = new URL("path/to/image.jpg", "https://gitlab.renkulab.io/namespace/name/-/raw/main/");
mediaUrl.toString() // 'https://gitlab.renkulab.io/namespace/name/-/raw/main/path/to/image.jpg'
const mediaUrl = new URL("s3://example/image.jpg", "https://gitlab.renkulab.io/namespace/name/-/raw/main/");
mediaUrl.toString() // 's3://example/image.jpg'

The URL constructor will handle relative vs absolute locations for you without needing to re-implement it.

@andre-code andre-code temporarily deployed to renku-ci-ui-2791 September 12, 2023 13:09 — with GitHub Actions Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2791 September 13, 2023 07:49 — with GitHub Actions Inactive
@andre-code andre-code temporarily deployed to renku-ci-ui-2791 September 13, 2023 08:14 — with GitHub Actions Inactive
@andre-code andre-code merged commit 33ae87f into master Sep 14, 2023
16 checks passed
@andre-code andre-code deleted the 2765-fix-dataset-avatar-url branch September 14, 2023 06:32
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

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.

Support avatars with external URLs
3 participants