Added page to show statistics from use of the workshop

[elisakiv]

Proposed changes

Created a statistics page to view statistics regariding the use of the workshop, such as reservations made, and when the workshop is in use.

Areas to review closely

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)

[codecov]

Codecov Report

Merging #669 (ab926dc) into dev (6f55b7d) will increase coverage by 0.04%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##              dev     #669      +/-   ##
==========================================
+ Coverage   88.16%   88.21%   +0.04%     
==========================================
  Files         152      152              
  Lines        6187     6218      +31     
==========================================
+ Hits         5455     5485      +30     
- Misses        732      733       +1     

Files	Coverage Δ
src/makerspace/urls.py	100.00% <ø> (ø)
src/makerspace/views.py	98.70% <96.87%> (-1.30%)	⬇️

[Gunvor4]

On the third plot, some of the sewing machines appear twice

[Gunvor4]

On a normal screen, with two plots next to each other, the title is above the second plot. It would look better if it was in the middle. The two plots are also not situated in the middle of the page, which is fine, but then the title of the page should be adjusted relative to the plots, and not the page. The same issue is still there when the screen size gets smaller and only one plot is shown per row.

[Gunvor4]

In the two plots counting how many times a 3D printer or a sewing machine has been booked by someone, it looks a bit strange that there are decimals on the y-axis, since it is impossible for a machine to be booked 0.5 times. The two other plot, on the other hand, are counting hours, so there it would have been ok to see decimals. But these plots only have integers on the y-axis. The styles should be switched – the "number of times" plots should not have decimals, and the "how many hours" plots, could.

[Gunvor4]

The last plot doesn't show any numbers on the y-axis, and the axis also doesn't have a label. I guess what is shown here is the result of a calculation? Still – would it be possible to give the y-axis a label or introduce some numbers here, to make it clearer what the plot actually shows? Maybe add a label like "equipment currently in use" or "machines or workstations currently in use"?

[Gunvor4]

On the x-axis on the last plot – the numbers are tilted, like for the names of 3D-printers or sewing machines. But on this plot, the only thing the x-axis shows are time points, which are shorts, so tilting them looks a bit strange.

[Gunvor4]

I like the colors of the plots. Still, it might be more logical if they were gradients made from MAKE's official colors, which are MAKE yellow (#F8C811), dark blue (#0C426A) and turquoise (#00EBB3), in addition to two different shades of gray - #4A4B4C and #DCDDDE. We also have an orange variation of the yellow (#F5A13F).
If you decide to keep the colors you already have, maybe the rainbow plot could be smoother, because in the second and fourth bar looks more orange, while the third and the fifth looks more yellow. Also – the ninth bar looks more purple and the tenth looks more blue, while the other plots seem to follow a gradient.
Also – if you have 10 colors in your gradient, and we have more than 10 3D printers, we would go from the 10th color and back to the first, which will not look too nice. Maybe the gradient could be calculated in the JavaScript file, from the first and the last color, and the number of bars? If not, it would also be possible to reverse the gradient, as I've shown in the example. In the example, I used two of the official MAKE colors, and created a gradient using the tool on this page.

Suggested change

	'rgba(249, 65, 68)',
	'#00EBB3',
	'#03C1A1',
	'#06978F',
	'#096C7C',
	'#0C426A',
	'#096C7C',
	'#06978F',
	'#03C1A1',

[Gunvor4]

The exprassions for reservations (line 80 – 82) sewingmachines (line 84 – 87) and printers (line 89 – 92) are extremely long and ugly. Would it be possible to refactor these, to make them more readable?

[Gunvor4]

I don't think you need a colon after the title of a plot ;-)

[Gunvor4]

It might be an idea to sort the printers, either alphabetically, or to keep the same order as they appear in on the website. Now I added some fake ultimakers, then some fake raise3D printers, a fake SLA printer, and then some fake ultimakers again, and now I have the ultimakers at both sides of the plot, with raise 3D printers and SLA printers in the middle.

[Gunvor4]

Aaaand now I'm done! :-) Sorry about being nitpicky :-P

[ddabble]

Amazing job! Properly displaying statistics is usually a pretty complex task, so I'm genuinely impressed by what you've created 😊 However, complexity also necessarily increases the risk of overlooking smaller details, and so I've found quite a few things to provide feedback on 😅 If you'd like some help making any of the suggested changes, or if you'd simply like to offload some work, I'd be happy to help :)

---

First off, it seems like no parts of the code you've added use any of the things in the makerspace app, and instead only use things in the make_queue app... I guess it makes sense you added it to the former app since all of the statistics relate to the physical makerspace (another reason why the two mentioned apps have pretty terrible names, in my opinion 😅), but the code should really be moved to the make_queue app. This will require a little work, but I'd absolutely recommend you do this, for future maintainability 🙂

It would be great if you could fix the issues detected by Code Climate (link is in the status checks at the bottom of this PR page) 🙂 (It should be mentioned that fixing auto-detected issues like those, might not necessarily make the code any better - it might even make the code worse in some cases - but they're usually a very good indication!) It looks like most of the issues can/will be fixed after addressing some of my comments below, though :)

Lastly, it would be fantastic if you could add some unit tests for the view code, to both make sure that it works the way we think it does, and to provide concrete examples for future developers, which can help them understand the intended function of the code 😊

Btw, well done minimizing the number of DB queries using prefetching! That's often quite a daunting task, but it looks like you've implemented it well! 😄

[ddabble]

Looks like you forgot to remove all the remnants of a merge conflict :p

Suggested change

>>>>>>> 66675ec2 (Added page to show statistics from use of the workshop)

[ddabble]

The abbreviation "pr." is not really used in English, as far as I'm aware 🤔 Also, there is no dash between "3D" and "printer" in English 🙂

Suggested change

	msgid "Number of reservations pr. 3D-printer"
	msgid "Number of reservations per 3D printer"

[ddabble]

Suggested change

	msgid "Number of reservations pr. sewing machine"
	msgid "Number of reservations per sewing machine"

[ddabble]

Suggested change

	msgid "Longest printer reservations"
	msgid "Longest 3D printer reservations"

[ddabble]

Suggested change

	msgstr "Lengste 3D-printer reservasjoner"
	msgstr "Lengste 3D-printerreservasjoner"

[ddabble]

Please use pk instead of id in your code wherever possible, as per the common code smells guide 🙂

Suggested change

	annotate(len=Sum((F('reservations__end_time') - F('reservations__start_time')) / self.get_time_in_hours, output_field=IntegerField())).values('id', 'name', 'len'))
	annotate(len=Sum((F('reservations__end_time') - F('reservations__start_time')) / self.get_time_in_hours, output_field=IntegerField())).values('pk', 'name', 'len'))

[ddabble]

This 3 is a magic number and should preferably be replaced with a field on the view (e.g. NUM_LONGEST_RESERVATIONS_DISPLAYED = 3) :)

Also, limiting querysets using the list slicing syntax should be done on the queryset itself before converting it to a list, like this:

Suggested change

	order_by('-len').values('id', 'name', 'reservations', 'len'))[:3]
	order_by('-len').values('id', 'name', 'reservations', 'len')[:3])

Otherwise, the entire queryset (probably thousands of rows) will be converted into a list first, and then sliced 😅

---

Lastly, correct me if I'm wrong, but doesn't the longest_printer_reservations queryset contain the three machines that have been reserved the longest duration in total, and not the three machines with the longest individual reservations, as suggested by the variable name..? (I haven't run the code locally, so I don't know what the graphs actually display 😅)

[ddabble]

Oh wait, after testing some of the code locally, it seems like longest_printer_reservations does indeed contain what the name suggests, and the annotated len attribute's value is, appropriately, the duration of each reservation - in contrast to the renaming suggestion I made in a comment above 😅 The code makes this pretty confusing, since each row/element of the list does not represent each machine - the opposite of what you'd expect, and what the lists above and below contain. (I'm explaining this to avoid any potential misunderstandings:) Instead, due to the way values() works, when providing the name of a many-to-many field or a reverse many-to-one manager ('reservations' in this case), it will return one row/element for each combination the multivalued fields provided (one row per reservation, in this case), as explained in the warning at the bottom of the values() section in the docs. Under the values_list() section, this is mentioned:

values() and values_list() are both intended as optimizations for a specific use case: retrieving a subset of data without the overhead of creating a model instance. This metaphor falls apart when dealing with many-to-many and other multivalued relations (such as the one-to-many relation of a reverse foreign key) because the “one row, one object” assumption doesn’t hold.

So, I think code relying on such an obscure fact of the values() method should be avoided (or at the very least explicitly pointed out and explained in comments). I think I found a fairly easy replacement, however, which also makes the query slightly less complex by going through the reservation table instead of the machine table: 🙂(This contains some elements from other parts of the code and from my other suggestions, as you can see :))

longest_printer_reservations = (
    Reservation.objects.filter(event=None, special=False, machine__machine_type__pk=1)
    .annotate(duration=F('end_time') - F('start_time'))
    .order_by('-duration')
    .select_related('machine')
    [:3]
)
longest_printer_reservations_list = []
for reservation in longest_printer_reservations:
    duration: timedelta = reservation.duration
    longest_printer_reservations_list.append({
        'machine_name': reservation.machine.name,
        'duration_hours': duration.total_seconds() // (60 * 60),
    })

longest_printer_reservations_list can then be inserted into the context variables 🙂 Any thoughts?

[ddabble]

The fact that the duration calculated here has a unit of microseconds, is not the case for all DBMSs (see for example the note in the Django docs about PostgreSQL). I think we should write code that is as DBMS-agnostic as practically possible, and so it would be better to remove this calculation from the query...

Suggested change

	span_of_printer_reservations = list(self.get_machines_with_reservations("3D-printere").
	annotate(len=Sum((F('reservations__end_time') - F('reservations__start_time')) / self.get_time_in_hours, output_field=IntegerField())).values('id', 'name', 'len'))
	span_of_printer_reservations = list(self.get_machines_with_reservations("3D-printere").
	annotate(len=Sum(F('reservations__end_time') - F('reservations__start_time'))).values('id', 'name', 'len'))

...and instead do it after, using timedelta.total_seconds():

for machine_dict in span_of_printer_reservations:
    total_reserved_duration: timedelta = machine_dict.pop('len')
    machine_dict['total_reserved_hours'] = total_reserved_duration.total_seconds() // (60 * 60)

This would be natural to place inside a helper method, of course.

If you decide to implement my suggestion above, then I guess the len attribute could be renamed to
total_reserved_duration (instead of total_reserved_hours, as I suggested above - since that name is inserted into machine_dict in my suggestion). Or any other more fitting name you might come up with 😊

[ddabble]

The code might be made slightly cleaner if you moved the list() and values() calls (or just one of them) to the context variable dict, like this: 🤔

Suggested change

	number_of_sewing_machine_reservations = list(self.get_machines_with_reservations("Symaskiner").
	annotate(number_of_reservations=Count('reservations')).values('id', 'name', 'number_of_reservations'))
	get_time_distribution = self.get_time_distribution()
	context.update({'span_of_printer_reservations': span_of_printer_reservations,
	'span_of_sewing_machine_reservations': span_of_sewing_machine_reservations,
	'longest_printer_reservations': longest_printer_reservations,
	'number_of_printer_reservations': number_of_printer_reservations,
	'number_of_sewing_machine_reservations': number_of_sewing_machine_reservations,
	number_of_sewing_machine_reservations = self.get_machines_with_reservations("Symaskiner").
	annotate(number_of_reservations=Count('reservations'))
	get_time_distribution = self.get_time_distribution()
	context.update({'span_of_printer_reservations': span_of_printer_reservations,
	'span_of_sewing_machine_reservations': span_of_sewing_machine_reservations,
	'longest_printer_reservations': longest_printer_reservations,
	'number_of_printer_reservations': number_of_printer_reservations,
	'number_of_sewing_machine_reservations': list(number_of_sewing_machine_reservations.values('id', 'name', 'number_of_reservations')),

[ddabble]

Use select_related() for foreign key fields, as explained in one of my comments above 🙂

Suggested change

	return Machine.objects.prefetch_related('machine_type', 'reservations').filter(Q(machine_type__name__icontains=machine_type) & filter_reservations).order_by('machine_type')
	return Machine.objects.select_related('machine_type').prefetch_related('reservations').filter(Q(machine_type__name__icontains=machine_type) & filter_reservations).order_by('machine_type')