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

Feature/uu course descriptions #1722

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from

Conversation

lukavdplas
Copy link
Contributor

Adds several corpora with UU course descriptions.

I've kept the old "humanities course descriptions" corpus, because that one uses a slightly different data format. That corpus is kept the same, but uses the same picture as the "updated" version.

Because these corpora are created for a separate server, there are a few minor changes to the frontend to configure the server in environment.ts:

  • The navbar will now show the appName that is configured
  • You can hide the corpus filters on the home page. This makes sense if, like here, the corpora on the server are very uniform so there is nothing to filter on.

Server configuration changes

If a server was using the hum_course_descriptions corpus, it must update the location of the source file. It's now located in the uu_course_descriptions module.

@lukavdplas lukavdplas added the affects-deployment changes that require an update in the deployment module label Dec 4, 2024
@lukavdplas lukavdplas requested a review from Meesch December 11, 2024 17:54
@lukavdplas lukavdplas marked this pull request as ready for review December 11, 2024 17:54
Copy link
Contributor

@Meesch Meesch left a comment

Choose a reason for hiding this comment

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

Looks very nice, especially the landing page with all the faculties: great job! Got some small stuff and some comments, I tested only using the web app on the server.

corpus = corpus_from_api(admin_client)
assert corpus['title'] == 'All faculties'

goal = '''- Begrijpen van de functie van testen in software.
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

'-': None,
}

def get_from(mapping: Mapping) -> Callable:
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using a more descriptive function name, like 'transform_from_dictionary'

}

LEVELS = {
1: 'Bachelor 1',
Copy link
Contributor

Choose a reason for hiding this comment

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

The transform doesn't work for the keys of this dict that are integers: levels are still shown as '1', '2', '3' instead of 'Bachelor 1'.

extractor=CSV('OMSCHRIJVING'),
es_mapping=keyword_mapping(enable_full_text_search=True),
results_overview=True,
search_filter=MultipleChoiceFilter(option_count=100),
Copy link
Contributor

Choose a reason for hiding this comment

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

This filter doesn't seem to show up on the web version of course explorer...

return lambda doc: doc['faculty'] == FACULTIES[faculty.upper()]


class _UUCourseDescriptionsFaculty(UUCourseDescriptions):
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this implementation of subcorpora!

* necessary if the app name already includes "i-analyzer"
*/
get showIAnalyzerInBrand() {
return ! this.appName.toLowerCase().includes('i-analyzer');
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the solution stylistically, but I don't know if this is the best way to implement naming conventions. I would either add some documentation explaining how the naming of apps works and how a developer should use those values, or just give two fields in the environment for Main app name and Secondary App name or something? Or is this meant to enforce that we have the I-analyzer name present at all times? :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-deployment changes that require an update in the deployment module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants