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

Routing for courses and course tabs #162

Open
wants to merge 11 commits into
base: summer21
Choose a base branch
from

Conversation

20rohanc
Copy link
Contributor

This edit creates different links that correspond to each course and its tabs. To demonstrate, I will provide 8 screenshots below; each screenshot corresponds to one of the tabs in two selected courses (two courses so that one may see the URLs are unique to each course).

Contents Page:
Screen Shot 2021-07-19 at 3 29 19 AM
Screen Shot 2021-07-19 at 3 29 27 AM

Users Page:
Screen Shot 2021-07-19 at 3 29 43 AM
Screen Shot 2021-07-19 at 3 30 12 AM

Grades Page:
Screen Shot 2021-07-19 at 3 29 51 AM
Screen Shot 2021-07-19 at 3 30 18 AM

Course Settings Page:
Screen Shot 2021-07-19 at 3 30 00 AM
Screen Shot 2021-07-19 at 3 30 26 AM

@20rohanc
Copy link
Contributor Author

Turns out I forgot to put the links in the screenshots I took, so here are retakes.
Screen Shot 2021-07-19 at 3 35 01 AM
Screen Shot 2021-07-19 at 3 34 54 AM
Screen Shot 2021-07-19 at 3 34 48 AM
Screen Shot 2021-07-19 at 3 34 41 AM
Screen Shot 2021-07-19 at 3 34 35 AM
Screen Shot 2021-07-19 at 3 34 20 AM

@karger
Copy link
Member

karger commented Jul 19, 2021

This is good progress, but here's a suggestion for a small refinement. Tim Berners Lee always said that the best urls are those you can write down on a napkin. your course links have very long opaque tokens. It would be better if a course link was e.g. http://nb2/course-name. This doesn't quite work because you may have two courses with the same name. But if you look at how other sites address this, they tend to take the "human readable" url and then attach a small disambiguating string For example medium uses the article title with a small suffix: medium.com/this-cambridge-life/the-cosmologist-modelling-the-universe-with-maths-ad65adea1f80 . I think it would be nice to replicate this approach.

@karger
Copy link
Member

karger commented Jul 19, 2021

Also it looks like package-lock.json got into the repository. It shouldn't be there; it should be removed and then put into gitignore so it doesn't get added again.

@20rohanc
Copy link
Contributor Author

I can definitely implement a similar system for nb. I'm curious how we should move forward with collisions though; if two courses share the same name and small tag, there could be an issue. One idea I had was that we could scrape for all existing courses with the given course name in our URL and if multiple courses are retrieved, we use the smallest necessary tag to differentiate the two (this would be very similar to how chaining occurs with hash functions).

There are potential issues to this workaround. One would be that a database with all courses created on nb would have to exist (otherwise, there might be collisions for only certain users, leading to different necessary differentiations for each user). It is possible this database already exists, and if so I would simply need to learn what axios command would retrieve it. Of course, accessing this database to generate URLs could become costly as the number of courses in existence increases.

The other issue I see here is that courses might have their URLs change as other courses with the same name are added. To me, a simple fix would be to have the tag length be variable, so courses that were created earlier maintain their shorter (perhaps nonexistent) tags, while courses that are created later and collide are forced to have longer tags.

I'll be working on creating URLs similar to the medium blog in the meantime, but I'm excited to hear your input/advice on collisions!

@karger
Copy link
Member

karger commented Jul 20, 2021 via email

@JumanaFM
Copy link
Member

Side note: please merge branch summer21 to your codebase and resolve the conflicts then PR again.
I will merge once we decide the approach for addressing links

@20rohanc
Copy link
Contributor Author

@JumanaFM I have merged the haystack summer21 branch into my repository and resolved the issues, but there is a bug now resulting in the list of enrolled courses not rendering on the left sidebar. When working in developer tools, I am receiving the following: "Proxy error: Could not proxy request /api/classes/create from 127.0.0.1:8080 to https://127.0.0.1:3000/ (ECONNREFUSED)," while my command line says that the socket.io module can't be found. I'm guessing that this means that I'm running the application on the wrong port, but I'm unsure what caused this when the code merged since none of the issues I saw related to this. Any pointers on this situation would be appreciated!

I updated my code with better named variables in some locations (The "jj" class you mentioned and a couple of the variables used in my routing code). I've pushed this merged and modified code into my forked summer21 branch.

@karger I've been looking through a few other systems for the links we can use for nb and I'd like to play around with implementation when the aforementioned bug is resolved. I'm still figuring out axios (and the database side of the code in general), and I wanted to know if there was a way to use the codebase to find databases pertaining to nb and the commands I would use to access those databases (so that I can get a list of all courses).

I also was curious if it is possible to modify a course object as it is stored in the database. I was thinking of experimenting with adding another field that would represent the url ID, so we could generate a course's unique url tag when it is created and just use that value instead of checking for conflicts every time the course is accessed.

@JumanaFM
Copy link
Member

@20rohanc Hi Rohan! Thank you for the great progress. For the issue you’re facing, you need to do migration for your DB ttps://sequelize.org/master/manual/migrations.html#running-migrations

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.

3 participants