Avatar fix (#362)

* removed "debugger" line in Header/author/index.tsx

* support for getOriginUrl() on remotes in IGitService

* Fixed avatar issues (#349, #287) using remote contributors and added avatar cache extension setting (default 1 hour)

* Always request github users API (when avatar cache expires) and check for modifications

* Updated version and CHANGELOG.md

* set default value for _enabled in log.ts

* Slightly improved the use of getOriginType() through getOriginUrl()

* CodeFactor fixes

Author: ole

CHANGELOG.md

@@ -1,3 +1,7 @@
+## Version 0.4.7
+- Always request github users API (when avatar cache expires) and check for modifications
+- Fixed avatar issues (#349, #287) using remote contributors and added avatar cache extension setting (default 1 hour)
+
 ## Version 0.4.6
 - Handle cases where folder/file names conflicts with brancch names [#205](https://github.com/DonJayamanne/gitHistoryVSCode/issues/205), [#340](https://github.com/DonJayamanne/gitHistoryVSCode/issues/340)
 - Adds support for multi-root workspace folders [#346](https://github.com/DonJayamanne/gitHistoryVSCode/issues/346)

browser/src/actions/results.ts

@@ -20,33 +20,6 @@ export const notifyIsFetchingCommit = createAction(Actions.IS_FETCHING_COMMIT);
 export const fetchedAvatar = createAction<Avatar[]>(Actions.FETCHED_AVATARS);
 export const fetchedAuthors = createAction<ActionedUser[]>(Actions.FETCHED_AUTHORS);
 
-// function buildQueryString(settings: ISettings): string {
-//     if (!settings) {
-//       return '/log';
-//     }
-//     const queryArgs = [];
-//     if (settings.searchText && settings.searchText.length > 0) {
-//       queryArgs.push(`searchText=${encodeURIComponent(settings.searchText)}`);
-//     }
-//     if (settings.pageIndex && settings.pageIndex > 0) {
-//       queryArgs.push(`pageIndex=${settings.pageIndex}`);
-//     }
-//     if (settings.selectedBranchType) {
-//       switch (settings.selectedBranchType) {
-//         case Branch.All: {
-//           queryArgs.push(`branchType=ALL`);
-//           break;
-//         }
-//         case Branch.Current: {
-//           queryArgs.push(`branchType=CURRENT`);
-//           break;
-//         }
-//       }
-//     }
-
-//     return `/log` + (queryArgs.length === 0 ? '' : `?${queryArgs.join('&')}`);
-//   }
-
 function getQueryUrl(store: RootState, baseUrl: string, args: string[] = []): string {
     const id = store.settings.id || '';
     const queryArgs = args.concat([`id=${encodeURIComponent(id)}`]);
@@ -60,54 +33,13 @@ export const actionACommit = (logEntry: LogEntry) => {
         return axios.post(url, logEntry);
     };
 };
-export const fetchAvatar = (user: ActionedUser) => {
-    // tslint:disable-next-line:no-any
-    return async (dispatch: Dispatch<any>, getState: () => RootState) => {
-        const state = getState();
-        const url = getQueryUrl(state, '/avatar', [`name=${encodeURIComponent(user.name)}`, `email=${encodeURIComponent(user.email)}`]);
-        axios.get(url)
-            .then(result => {
-                dispatch(fetchedAvatar([result.data as Avatar]));
-            })
-            .catch(err => {
-                // tslint:disable-next-line:no-debugger
-                console.error('Git History: Avatar request failed');
-                console.error(err);
-            });
-    };
-};
-// export const fetchAvatars = (users: ActionedUser[]) => {
-//     // tslint:disable-next-line:no-any
-//     return async (dispatch: Dispatch<any>, getState: () => RootState) => {
-//         const state = getState();
-//         const url = getQueryUrl(state, '/avatar', [`name=${encodeURIComponent(user.name)}`, `email=${encodeURIComponent(user.email)}`]);
-//         axios.get(url)
-//             .then(result => {
-//                 dispatch(fetchedAvatar(result.data as Avatar));
-//             })
-//             .catch(err => {
-//                 // tslint:disable-next-line:no-debugger
-//                 console.error('Git History: Avatar request failed');
-//                 console.error(err);
-//             });
-//     };
-// };
+
 // tslint:disable-next-line:no-any
-export const fetchAvatars = async (users: ActionedUser[], dispatch: Dispatch<any>, getState: () => RootState) => {
+export const fetchAvatars = async (dispatch: Dispatch<any>, getState: () => RootState) => {
     const state = getState();
-    const unidentifiedUsers = users
-        .filter(a => !state.avatars.find(avatar => avatar.name === a.name && avatar.email === a.email))
-        .reduce((accumulator, user) => {
-            if (accumulator.findIndex(item => item.email === user.email && item.name === user.name) === -1) {
-                accumulator.push(user);
-            }
-            return accumulator;
-        }, []);
-    if (unidentifiedUsers.length === 0) {
-        return;
-    }
     const url = getQueryUrl(state, '/avatars');
-    axios.post(url, unidentifiedUsers)
+
+    axios.post(url)
         .then(result => {
             dispatch(fetchedAvatar(result.data as Avatar[]));
         })
@@ -270,19 +202,10 @@ function fetchCommits(dispatch: Dispatch<any>, store: RootState, pageIndex?: num
             if (Array.isArray(result.data.items)) {
                 result.data.items.forEach(item => {
                     fixDates(item);
-                    /*if (Array.isArray(item.committedFiles)) {
-                        item.committedFiles.forEach(f => {
-                            fixFileUri(f.oldUri);
-                            fixFileUri(f.uri);
-                        });
-                    }*/
                 });
             }
-            //fixFileUri(result.data.file);
             dispatch(addResults(result.data));
-            if (result.data && Array.isArray(result.data.items) && result.data.items.length > 0) {
-                fetchAvatars(result.data.items.map(item => item.author), dispatch, () => store);
-            }
+            fetchAvatars(dispatch, () => store);
         })
         .catch(err => {
             // tslint:disable-next-line:no-debugger
@@ -298,17 +221,9 @@ function fetchCommit(dispatch: Dispatch<any>, store: RootState, hash: string) {
         .then((result: { data: LogEntry }) => {
             if (result.data) {
                 fixDates(result.data);
-                /*if (Array.isArray(result.data.committedFiles)) {
-                    result.data.committedFiles.forEach(f => {
-                        fixFileUri(f.oldUri);
-                        fixFileUri(f.uri);
-                    });
-                }*/
             }
             dispatch(updateCommit(result.data));
-            if (result.data && result.data.author) {
-                fetchAvatars([result.data.author], dispatch, () => store);
-            }
+            fetchAvatars(dispatch, () => store);
         })
         .catch(err => {
             // tslint:disable-next-line:no-debugger

browser/src/components/Header/author/index.tsx

@@ -38,7 +38,7 @@ export class Author extends React.Component<AuthorProps, AuthorState> {
         if (this.props.lineHistory) {
             return null;
         }
-        debugger;
+        
         const title = !this.props.author || this.props.author === '' ? 'All Authors' : this.props.author;
         const selectedAuthor = !this.props.author || this.props.author === '' ? '[ALL]' : this.props.author;
         const authors = Array.isArray(this.props.authors) ? this.props.authors : [];

browser/src/components/LogView/Commit/Avatar/index.tsx

@@ -12,7 +12,7 @@ type AvatarProps = {
 function Avatar(props: AvatarProps) {
     let avatarUrl = '';
     if (props.result) {
-        const avatar = props.avatars.find(item => item.name === props.result.name && item.email === props.result.email);
+        const avatar = props.avatars.find(item => item.name === props.result.name || item.login === props.result.name || item.email === props.result.email);
         avatarUrl = avatar ? avatar.avatarUrl : '';
     }
     if (avatarUrl) {

package.json

@@ -2,7 +2,7 @@
     "name": "githistory",
     "displayName": "Git History",
     "description": "View git log, file history, compare branches or commits",
-    "version": "0.4.6",
+    "version": "0.4.7",
     "publisher": "donjayamanne",
     "author": {
         "name": "Don Jayamanne",
@@ -334,6 +334,12 @@
                     "default": 100,
                     "scope": "window",
                     "description": "Default number of items to be displayed in Git History Viewer"
+                },
+                "gitHistory.avatarCacheExpiration": {
+                    "type": "integer",
+                    "default": 60,
+                    "scope": "window",
+                    "description": "Avatar image cache expiration (default: 60 minutes / 0 = cache disabled)"
                 }
             }
         }

src/adapter/avatar/base.ts

@@ -2,63 +2,44 @@ import { injectable, unmanaged } from 'inversify';
 import { IStateStore, IStateStoreFactory } from '../../application/types/stateStore';
 import { IWorkspaceService } from '../../application/types/workspace';
 import { IServiceContainer } from '../../ioc/types';
-import { ActionedUser, Avatar } from '../../types';
+import { Avatar, AvatarResponse, IGitService } from '../../types';
 import { GitOriginType } from '../repository/types';
 import { IAvatarProvider } from './types';
 
-type CachedAvatar = Avatar & { retry: boolean; dateTimeMs: number };
-type CachedPromise = { dateTimeMs: number; promise: Promise<Avatar | undefined> };
 @injectable()
 export abstract class BaseAvatarProvider implements IAvatarProvider {
     protected readonly httpProxy: string;
     private readonly avatarStateStore: IStateStore;
-    private readonly getAvatarPromises: Map<string, CachedPromise>;
     public constructor(protected serviceContainer: IServiceContainer, @unmanaged() private remoteRepoType: GitOriginType) {
         const workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
         this.httpProxy = workspace.getConfiguration('http').get('proxy', '');
         const stateStoreFactory = this.serviceContainer.get<IStateStoreFactory>(IStateStoreFactory);
         this.avatarStateStore = stateStoreFactory.createStore();
-        this.getAvatarPromises = new Map<string, CachedPromise>();
     }
-    public async getAvatar(user: ActionedUser): Promise<Avatar | undefined> {
-        const key = `Git:Avatar:${user.name}:${user.email}`;
-        const cachedInfo = this.avatarStateStore.has(key) ? await this.avatarStateStore.get<CachedAvatar>(key)! : undefined;
-        const retry = cachedInfo && cachedInfo.retry && (cachedInfo.dateTimeMs + (60 * 60 * 1000) < new Date().getTime());
-        if (!cachedInfo || retry) {
-            try {
-                // If we're in the process of getting the avatar for a user, then don't request it again.
-                // This way we avoid multiple requests for the same user (minimize 403 errors).
-                let cachedPromise: CachedPromise | undefined;
-                if (this.getAvatarPromises.has(key)) {
-                    cachedPromise = this.getAvatarPromises.get(key)!;
-                    if (cachedPromise.dateTimeMs + (60 * 60 * 1000) < new Date().getTime()) {
-                        this.getAvatarPromises.delete(key);
-                        cachedPromise = undefined;
-                    }
-                }
-                if (!cachedPromise) {
-                    cachedPromise = { dateTimeMs: Date.now(), promise: this.getAvatarImplementation(user) };
-                    this.getAvatarPromises.set(key, cachedPromise);
-                }
-                const avatar = await cachedPromise!.promise;
-                // const avatar = await this.getAvatarImplementation(user);
-                const url = avatar ? avatar.url : undefined;
-                const avatarUrl = avatar ? avatar.avatarUrl : undefined;
-                const dateTimeMs = new Date().getTime();
-                // If we don't have any errors, then never retry to get avatar, even if it is null.
-                await this.avatarStateStore.set<CachedAvatar>(key, { url, retry: false, avatarUrl, dateTimeMs, name: user.name, email: user.email });
-            } catch (ex) {
-                // If we have errors in retreivig the info, then we can retry later.
-                const dateTimeMs = new Date().getTime();
-                await this.avatarStateStore.set<CachedAvatar>(key, { url: undefined, retry: true, avatarUrl: undefined, dateTimeMs, name: user.name, email: user.email });
-            }
+
+    public async getAvatars(repository: IGitService) : Promise<Avatar[]> {
+        const workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
+        const cacheExpiration = workspace.getConfiguration('gitHistory').get<number>('avatarCacheExpiration', 60); // in minutes (zero to disable cache)
+
+        const remoteUrl = await repository.getOriginUrl();
+        const key = `Git:Avatars:${remoteUrl}`;
+
+        const cachedAvatars = await this.avatarStateStore.get<AvatarResponse>(key);
+
+        const retry = cacheExpiration === 0 || !cachedAvatars || (cachedAvatars && cachedAvatars.timestamp && (cachedAvatars.timestamp + (cacheExpiration * 60 * 1000)) < new Date().getTime());
+
+        if (retry) {
+            const avatars = await this.getAvatarsImplementation(repository);
+            await this.avatarStateStore.set<AvatarResponse>(key, { timestamp: new Date().getTime(), items: avatars });
+            return avatars;
+        } else if (cachedAvatars) {
+            return cachedAvatars.items;
         }
 
-        const info = await this.avatarStateStore.get<Avatar>(key)!;
-        return info && info.avatarUrl ? info : undefined;
+        return [];
     }
     public supported(remoteRepo: GitOriginType): boolean {
         return remoteRepo === this.remoteRepoType;
     }
-    protected abstract getAvatarImplementation(user: ActionedUser): Promise<Avatar | undefined>;
+    protected abstract getAvatarsImplementation(repository: IGitService) : Promise<Avatar[]>;
 }