-
Notifications
You must be signed in to change notification settings - Fork 0
[#ST-78] add learning nuggets app #6
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
Conversation
apps/learning_nuggets/views.py
Outdated
|
|
||
| class LearningCenterView(ListView): | ||
| """Main learning center page that shows all categories""" | ||
| template_name = 'learning_center_index.html' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hom3mad3 @partizipation templates need to be in a directory that follows the app label, and give that path here. See e.g in meinberlin, the templates e.g for the project app, are found under templates/meinberlin_project/, where the name derives from the project label meinberlin + app. For the a+ forks, I guess there was an agreement to call the fork projects as a4_candy
12809b2 to
c4f2f33
Compare
m4ra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I fixed the few missing bits for the tests. I need to run it locally too. Let me know if the error you were getting still persists.
| }, | ||
| bases=("wagtailcore.page",), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hom3mad3 I squashed them in one file, by deleting them all, and recreating them.
c4f2f33 to
bf4db85
Compare
adhocracy-plus/config/urls.py
Outdated
|
|
||
| # generic patterns at the very end | ||
| urlpatterns += [ | ||
| path('learning-center/', include('apps.learning_nuggets.urls', namespace='learning_nuggets')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might need to be moved up, however since i'm overwriting the urls of wagtail pages, would need to look into it in more detail and i didn't have the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends how the URL should be constructed. Should it be below the wagtail'sadmin/ path or below the domain name? If they work as they are, I would leave them for now.
| @@ -0,0 +1,11 @@ | |||
| import rules | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m4ra kept it simple, we might need to confirm what actually needs to be shown to whom. also couldn't simulate, since I realized don't know how to create users with the respective permissions hehe 🙃
20a5963 to
165f59d
Compare
m4ra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Need to check why tests are failing. For the extra test as initiator, did you have a look at projects tests? There are similar tests,
adhocracy-plus/config/urls.py
Outdated
|
|
||
| # generic patterns at the very end | ||
| urlpatterns += [ | ||
| path('learning-center/', include('apps.learning_nuggets.urls', namespace='learning_nuggets')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends how the URL should be constructed. Should it be below the wagtail'sadmin/ path or below the domain name? If they work as they are, I would leave them for now.
apps/cms/blocks.py
Outdated
| description = CharBlock( | ||
| max_length=500, | ||
| required=False, | ||
| help_text="Please insert a short description of the video " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help text need the translation function _ from gettext_lazy
see e.g here
| {% if value.media.url|lower|slice:"-3:" == "mp3" or value.media.url|lower|slice:"-3:" == "wav" %} | ||
| <audio controls class="w-full" {% if value.title %}aria-labelledby="media-title"{% endif %} {% if value.description %}aria-describedby="media-description"{% endif %}> | ||
| <source src="{{ value.media.url }}" type="audio/{{ value.media.url|lower|slice:"-3:" }}"> | ||
| <a href="{{ value.media.url }}" class="underline focus:ring-2 focus:ring-blue-500"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change all the trans to translate? it is the preferred method, and we had changed most of them in other projects.
| elif media_file.endswith(".wav"): | ||
| return "audio/wav" | ||
| else: | ||
| return "type invalid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we have a thorough check for the file type here? to be on the safer side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll look at this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added extra file types. I'm wondering if you think it is a good idea to extend the Wagtail document model in order to validate the real content of the file - is this a security concern we have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to have audio/mpeg as this is more correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return "application/octet-stream" instead of type invalid?
apps/learning_nuggets/blocks.py
Outdated
| title = blocks.CharBlock( | ||
| required=True, | ||
| max_length=100, | ||
| help_text="Enter a clear, concise title for this learning nugget (max 100 characters recommended)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, and all help_text fields, need the gettext_lazy _
Also the length of the line should be 79 I think, we try to break longer lines in two with the use of brackets. See the example am linking above for this too.
| def __call__(self, request): | ||
| request.is_ajax = request.headers.get('X-Requested-With') == 'XMLHttpRequest' | ||
| response = self.get_response(request) | ||
| return response No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noice
6df7eb1 to
9be14f9
Compare
Description
This PR introduces the new Learning Nuggets App, which includes the Learning Center. Admins can now add learning categories via Wagtail snippets, and each category can contain individual nuggets.
learning-nuggets-720.mov
Learning Center: The Learning Center is a singleton page that can be added under the homepage in Wagtail. It supports custom slugs for each category, which define the category's URL. Each category is linked to custom permissions, allowing content access to be controlled based on roles (Moderator, Initiator, Participant).
Video Block: Repurposed a Video Block from the Liquid website and added it to the apps/cms module. This block is now used in the Learning Nuggets to embed videos alongside the learning content, enhancing interactivity and flexibility.
AJAX Handling: Added AJAX handling to load sidebar content dynamically without refreshing the page, while still maintaining a traditional SSR fallback in case JavaScript is not supported. The /learning-center page is available under its URL, while the sidebar renders content dynamically from anywhere on the page. By adding AJAX handling, I’m offering an alternative to our current approach of using React for everything. This demonstrates a more sustainable approach, leveraging Django to handle the heavy lifting while still providing dynamic functionality.
Modularity: introduced an /assets folder at the app level for better modularity. This is a suggestion for how we should structure our assets moving forward, as it aligns with best practices. We should discuss this further, but I believe it will make managing assets in a more scalable and organized way easier as the app grows.
Still a few things on my list to finish up, but this should give us a solid foundation for testing this feature ✨
Let me know if you have any questions or feedback!
To do
config/urls.pyto follow the standard organisation convention. (tests are failing)Lower prio stuff I didn't get around to doing yet (to be addressed in future PRs?)
Tasks
Notes
@m4ra i was unable to simulate users with moderator and initiator roles locally (i don't think I know well enough about the different permissions and how they get created). In
rules.py, I'm calling some predefined rules for organisations and projects, but we definitely need to double check and write a few tests.