Feature/lore wiki page

[Gunvor4]

Proposed changes

• Added a MAKE lore wiki to the internal page, so members can write wiki articles about anything lore in the MAKE universe.

Areas to review closely

Deployment notes

• After deployment, permissions for the general MAKE group should be changed, so every member of MAKE is able to both view, add, change and delete lore articles.

Checklist

(If any of the points are not relevant, mark them as checked)

• Remembered to run the makemigrations, makemessages and compilemessages management commands and committed any changes that should be included in this PR
• Created tests that fail without the changes, if relevant/possible
• Manually tested that the website UI works as intended with different device layouts
  • (Most common is to test with typical screen sizes for mobile (320-425 px), tablet (768 px) and desktop (1024+ px), which can easily be done with your browser's dev tools)
• Manually tested that everything works as intended when logged in as different users locally
  • (This can be e.g. anonymous users (i.e. not being logged in), "normal" non-member users, members of different committees, and superusers)
• Made sure that your code conforms to the code style guides
  • (It's not intended that you read through this whole document, but that you get yourself an overview over its contents, and that you use it as a reference guide / checklist while taking a second look at your code before opening a pull request)
• Attempted to minimize the number of common code smells
  • (See the comment for the previous checkbox)
• Added sufficient documentation - e.g. as comments, docstrings or in the README, if suitable
• Added your changes to the "Unreleased" section of the changelog - mainly the changes that are of particular interest to users and/or developers, if any
• Added a "Deployment notes" section above, if anything out of the ordinary should be done when deploying these changes to the server
• Structured your commits reasonably
  • (Some tips on this can be found in "Write Better Commits, Build Better Projects" and in the code style guides)
  • (Restructuring commits after they've already been committed, can be done using e.g. interactive rebase; see the Git docs on rewriting history)

[Gunvor4]

[codecov]

Codecov Report

Merging #675 (4d9e2a4) into dev (6f55b7d) will decrease coverage by 0.55%.
The diff coverage is 56.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #675      +/-   ##
==========================================
- Coverage   88.16%   87.62%   -0.55%     
==========================================
  Files         152      152              
  Lines        6187     6288     +101     
==========================================
+ Hits         5455     5510      +55     
- Misses        732      778      +46     

Files	Coverage Δ
src/internal/admin.py	100.00% <100.00%> (ø)
src/internal/urls.py	100.00% <100.00%> (ø)
src/internal/models.py	90.39% <58.82%> (-3.40%)	⬇️
src/internal/views.py	85.14% <64.15%> (-5.77%)	⬇️
src/internal/forms.py	75.18% <31.03%> (-11.98%)	⬇️

[elisakiv]

It looks great, just few comments:

It seems that when the title of an article is one very long word with no spaces, it behaves strangely on mobile. The name does not wrap when it becomes too long so the title continues on one line, past the length of the header.

When the title is long, but does contain spaces, the placement of the edit and delete button is a little awkward.

[elisakiv]

If a users adds an article with a name only consiting of special character, the page crashes.

[elisakiv]

When editing an article with a picture, the button and checkbox for removing the picture behaves a little strangely. The checkbox is not level with the picure name, and the "Clear" button does not fully behave like a button, mainly because the cursor does not become a hand, the user does not receive any feedback that the button was clicked, and it does not look like a button either, it looks like the other text on the page.

[elisakiv]

If the article has a very long name, the title in the menu overlaps with the text of the article that the user has open

[ddabble]

Very nice job! 🤩 Just a few comments 😅

I guess I could comment more on the CSS, but I can potentially wait with that until after you've made some of the changes I suggested below - if you decide to 🙂 (Otherwise, I must admit that I personally don't find CSS code in general to be particularly important to keep polished :))

Finally, it would also be very nice if you could add some (simple) tests for the new code, so that we can feel more secure that it keeps working the way we think it does - both now and after future changes to the codebase 🙂

[ddabble]

Just doing a second round after the most recent commits 🙂

I just remembered that I might not have taught you anything about writing tests..? If so, just send a DM to me (or maybe @elisakiv..?) if you'd like some pointers :)

In any case, I wanted to reiterate that it would be great if you could add some (simple) tests for the lore pages, which would both help us feel more secure that the code behaves as we think it does - especially in edge cases - and help future developers understand the intended (main) functionality of the code by showing some typical scenarios in some of the tests - as tests often do. Examples include:

• That a MAKE member (i.e. a user with some of the lore permissions) is able to view/add/change(/delete?) lore pages (through GET and/or POST requests), but an unprivileged user is not
• That the slug field has the expected value after submitting various titles
• That colliding slugs/titles are correctly identified
• That the slug/title field cannot be empty
• Anything else that you think would be useful to have tests for 🙂

[ddabble]

As explained at the bottom of this comment, functions like get_context_data() with a **kwargs parameter can preferably be structured like this: 🙂

    def get_context_data(self, **kwargs):
        return {
            <other context variables>
            **super().get_context_data(**kwargs),
        }

So:

Suggested change

	def get_context_data(self, **kwargs):
	context = super().get_context_data(**kwargs)
	context['show_article'] = True
	context['all_lore_articles'] = Lore.objects.order_by('title')
	return context
	extra_context = {
	'show_article': True,
	}
	def get_context_data(self, **kwargs):
	return {
	'all_lore_articles' = Lore.objects.order_by('title'),
	**super().get_context_data(**kwargs),
	}

('show_article' could also have been placed inside the dict returned in get_context_data(), but I think it's better to have it match the way it's inserted in LoreListView.)

[Gunvor4]

I saw the previous comment, but frankly I don't understand the advantages of doing it the way you propose, so it's a bit frustrating to have to change something that works when I don't understand why I am changing it. I have now changed it, but I'd appreciate an explanation to why the second version of this code is better than the first.

[ddabble]

Sure, that's fair 🙂 So in this case, I guess the explanation has two parts (I'm guessing you were mostly interested in the first half of the second one..?):

1. "Why move context variables from get_context_data() to extra_context?"

   This is generally because there's less "noise" when reading it (compare e.g. extra_context = {...} to def get_context_data(self, **kwargs): return {...}), and so it's preferable whenever possible - which is the case when the values are constant (e.g. True), but not when they're variable/dynamic (e.g. Lore.objects.order_by('title')).

   In this case, however, one of the values is constant, while the other is variable, and so having both extra_context and get_context_data() can make the code even noisier, but it can also be worth keeping the constant and variable variables separated, for the sake of organization - it's hard to say that either approach will always be the best choice 🤷 But since LoreListView already has an extra_context with 'show_article', I think it's best to have its "sister view", LoreDetailView, match that, if that makes sense :)

2. "Why structure the code inside get_context_data() the way I suggested?"

   This is purely to follow a principle of designing your code to be as future-proof as practically possible (i.e. as long as it doesn't take substantially more effort compared to a "naive" design). To illustrate why I think my suggestion adheres more closely to that principle, take the following example of code that could potentially be written in the future:

   • Writing a test for how the view and/or template behaves when some of the context variables are different (for whatever reason):

     lore_detail_view_instance.get_context_data(show_article=False, all_lore_articles=None)

     This works with the code I suggested, but with the original code, 'show_article' would have been overwritten (or "forced") to always have the value True regardless of the kwargs provided - and likewise for 'all_lore_articles' being overwritten to equal Lore.objects.order_by('title').

   That's what I meant when I said "[...] always let other code calling a context data method to be able to override the values set in the method, using kwargs" in the other comment.

   Also, as an added bonus, PyCharm seems to have a feature of providing code suggestions in templates for the context variables whose names are directly mentioned in the dict literal (i.e. {...}) returned by get_context_data() - but only when we write return {...} and not e.g. context = {...} followed by return context. (This is most likely because it's easier to parse one dict literal you know the location of (i.e. directly following the method's sole return statement), than to somehow analyze which context variables are inserted into context and then later returned 🤷)

Did that explanation make sense? If not, which details was I e.g. too vague about? 🙂

[ddabble]

As mentioned in a comment above, this (and the same method of LoreUpdateView) can be changed to the following after adding get_absolute_url() to Lore:

Suggested change

	title = str(self.object)
	title = title.replace('ø', 'o')
	title = title.replace('æ', 'ae')
	slug = slugify(title)
	return reverse_lazy('lore_detail', args=[slug])
	return self.object.get_absolute_url()

Also, since both the mentioned views' implementations of get_success_url() are completely identical, they can be removed and instead moved to LoreFormMixin 🙂

[ddabble]

This should be possible to replace with:

Suggested change

	title = str(self.get_form_kwargs()['instance'])
	title = title.replace('ø', 'o')
	title = title.replace('æ', 'ae')
	slug = slugify(title)
	return reverse_lazy('lore_detail', args=[slug])
	return self.get_success_url()

[ddabble]

Perhaps it would be nice to have the back button simply say the title of the article you're editing? 🤔 What do you think?

Suggested change

	back_button_text = _("Lore Article")
	def get_back_button_text(self):
	return self.object.title

[ddabble]

This change can be reset :)

[ddabble]

Btw, regarding the deployment notes in the PR description, it might be wise to not let all members of MAKE delete lore articles, and instead only grant that permission to e.g. the board members - similar to the group permissions for the internal quotes currently on the website 🤔 What do you think?