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

Support multi language filter track/session/room #239

Merged
merged 16 commits into from
Sep 29, 2024

Conversation

odkhang
Copy link
Collaborator

@odkhang odkhang commented Sep 25, 2024

Summary by Sourcery

Implement multi-language support for filtering tracks, sessions, and rooms in the schedule view, allowing users to filter based on their preferred language settings.

New Features:

  • Add support for multi-language filtering of session types, tracks, and rooms in the schedule view.

Enhancements:

  • Enhance the filtering logic to handle session types that are represented as either strings or objects, allowing for more flexible filtering based on user-selected languages.

Copy link

sourcery-ai bot commented Sep 25, 2024

Reviewer's Guide by Sourcery

This pull request implements multi-language support for filtering tracks, sessions, and rooms in the schedule view. The changes primarily affect the filtering logic and data preparation for the filter components, allowing for both string and object representations of session types, track names, and room names to accommodate multiple languages.

File-Level Changes

Change Details Files
Implement multi-language support for session type filtering
  • Add logic to handle both string and object representations of session types
  • Implement language preference handling, with fallback to English or the first available language
  • Create a Set to store unique session types across all languages
  • Update the filter data preparation to use the new multi-language session types
webapp/src/views/schedule/index.vue
webapp/src/views/schedule/sessions/index.vue
Add multi-language support for tracks and rooms filtering
  • Create a new filterLanguage method to handle multi-language track and room names
  • Implement language preference handling, with fallback to English or the first available language
  • Update the filter data preparation to use the new filterLanguage method for tracks and rooms
webapp/src/views/schedule/index.vue
webapp/src/views/schedule/sessions/index.vue
Modify the filtering logic to support multi-language data
  • Update the filtering logic in computed properties to handle both string and object representations of session types
  • Modify the filter method to use the new multi-language data structures
webapp/src/views/schedule/index.vue
webapp/src/views/schedule/sessions/index.vue
Add utility method for getting user language preference
  • Create a getLanguage method to retrieve the user's language preference from localStorage
  • Use the getLanguage method in various parts of the code to ensure consistent language handling
webapp/src/views/schedule/index.vue
webapp/src/views/schedule/sessions/index.vue

Sequence Diagram

sequenceDiagram
    participant User
    participant Component
    participant FilterMethod
    participant LanguageUtil
    User->>Component: Apply filter
    Component->>FilterMethod: Call filter method
    FilterMethod->>LanguageUtil: Get user language
    LanguageUtil-->>FilterMethod: Return user language
    FilterMethod->>FilterMethod: Process multi-language data
    FilterMethod-->>Component: Return filtered results
    Component-->>User: Display filtered content
Loading

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @odkhang - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider refactoring to reduce code duplication between files. Extracting common logic into shared functions could improve maintainability.
  • The language selection logic is complex and nested. Consider breaking it down into smaller, well-named functions to improve readability and ease of maintenance.
  • If possible, consider standardizing the data structure upstream to simplify the type checking and conditional logic in this code. If not feasible, adding type guards or helper functions could make handling different data shapes more elegant.
Here's what I looked at during the review
  • 🟡 General issues: 4 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

filter.tracks.data = this.schedule.tracks.map(t => { t.value = t.id; t.label = t.name; return t })
filter.rooms.data = this.schedule.rooms.map(t => { t.value = t.id; t.label = t.name; return t })
filter.types.data = this.schedule.session_type.map(t => { t.value = t.session_type; t.label = t.session_type; return t })
const setSessionType = new Set()
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider refactoring the filter() computed property for better readability and maintainability

The current implementation of the filter() computed property, while functional, is quite complex and hard to read. Consider breaking down the logic into smaller, more focused helper functions. This would improve readability, maintainability, and testability of the code.

getLanguage() {
return localStorage.getItem('userLanguage') || 'en'
},
filterLanguage(data) {
Copy link

Choose a reason for hiding this comment

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

suggestion: Simplify the filterLanguage function using functional programming concepts

The current implementation of filterLanguage uses complex nested loops and conditions. Consider simplifying this logic using functional programming concepts, such as Array.prototype.find or Array.prototype.reduce. This could make the code more concise and easier to understand.

		filterLanguage(data) {
			const userLanguage = this.getLanguage();
			return data.reduce((acc, item) => {
				const translation = item.translations.find(t => t.language === userLanguage) 
					|| item.translations.find(t => t.language === 'en');
				if (translation) {
					acc.push({ ...item, ...translation });
				}
				return acc;
			}, []);
		},

@@ -154,9 +154,26 @@
let founds = null
if (selectedIds.length) {
if (results && results.length) {
founds = self.schedule.talks.filter(t => selectedIds.includes(t[refKey]) && results && results.includes(t.id))?.map(i => i.id) || []
founds = self.schedule.talks.filter(t => {
Copy link

Choose a reason for hiding this comment

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

suggestion: Refactor the session_type handling in applyFilter for improved readability

The logic for handling session_type in the applyFilter method has become quite complex. Consider extracting the session_type handling into a separate function or using early returns to improve readability. Also, there's some repetition between the two conditions (with and without results) that could be reduced.

						founds = self.schedule.talks.filter(t => {
							const sessionTypeMatches = this.checkSessionTypeMatch(t, refKey);
							return selectedIds.includes(t[refKey]) && (!results || results.includes(t.id)) && sessionTypeMatches;
						}).map(i => i.id);

						checkSessionTypeMatch(talk, refKey) {
							if (refKey !== 'session_type') return true;
							const sessionType = talk?.session_type;
							return typeof sessionType === 'string' ? selectedIds.includes(sessionType) : selectedIds.some(id => sessionType?.includes(id));
						},

@@ -296,6 +349,49 @@
},
resetOnlyFavs() {
this.onlyFavs = false
},
getLanguage() {
Copy link

Choose a reason for hiding this comment

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

suggestion: Extract common logic into a shared utility or mixin to reduce duplication

There's significant code duplication between webapp/src/views/schedule/index.vue and webapp/src/views/schedule/sessions/index.vue. Consider extracting the common logic (such as getLanguage, filterLanguage, and the internationalization logic in filter) into a shared utility function or a mixin. This would improve maintainability and ensure consistency across components.

import { getLanguage } from '@/utils/languageUtils'

// ...

methods: {
  // ...
  getLanguage,
  // ...
}

Copy link
Member

@hongquan hongquan left a comment

Choose a reason for hiding this comment

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

Please follow sourcery-ai suggestions to split to smaller functions, make code less nested, easier to read and debug later.

@odkhang odkhang requested a review from hongquan September 27, 2024 06:59
@odkhang
Copy link
Collaborator Author

odkhang commented Sep 27, 2024

Hi @hongquan, complex function is split to smaller function, please help us to review it again.

@mariobehling mariobehling merged commit 21af6a9 into fossasia:development Sep 29, 2024
2 of 3 checks 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.

4 participants