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

Rework invitations backend #185

Merged
merged 1 commit into from
Oct 31, 2021
Merged

Rework invitations backend #185

merged 1 commit into from
Oct 31, 2021

Conversation

IidaKainu
Copy link
Collaborator

  • Add timestamp and unique constraint (combination of roadmapId & email) to invitations
  • Add route for getting invitations

Comment on lines 33 to 37
json = super.$parseDatabaseJson(json);
const date = json.updatedAt && new Date(json.updatedAt);
json.updatedAt = date;
json.valid = date >= moment().subtract(48, 'hours').toDate();
return json;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had a bit of trouble comparing the date: went with a solution similar to what's described here: Vincit/objection.js#91 (comment)

@PatricKangasmaki
Copy link
Collaborator

Changes are looking good 👍 But I'm not sure how I feel about using moment.js, though. Their official page is saying:

Moment.js is a legacy project, now in maintenance mode. In most cases, you should choose a different library.

Moment was built for the previous era of the JavaScript ecosystem. The modern web looks much different these days. Moment has evolved somewhat over the years, but it has essentially the same design as it did when it was created in 2011.

Another common argument against using Moment in modern applications is its size. Moment doesn't work well with modern "tree shaking" algorithms, so it tends to increase the size of web application bundles. If one needs internationalization or time zone support, Moment can get quite large.

Recently, Chrome Dev Tools started showing recommendations for replacing Moment for the size alone. We generally support this move.

More information: https://momentjs.com/docs/#/-project-status/

I would probably pick some other library, maybe even one of the libraries they suggest (or go without a library and work with js only if nothing too complicated is needed):
https://momentjs.com/docs/#/-project-status/recommendations/

If the date is only used in substracting time, you could use these techniques without additional libraries. There's also a good discussion on daylight savings times etc.
https://stackoverflow.com/questions/1296358/how-to-subtract-days-from-a-plain-date

Instead of 48 hours, subtract 2 days and instead of a month, subtract 30 days. This would also get rid of a problem where sometimes month would be 28 days and sometimes 31.

@IidaKainu
Copy link
Collaborator Author

Thank you for sharing the info about moment.js! 🙏 I have not worked much with js dates, so I had no idea.
Since there's only a few places where date subtraction is needed I went with the plain js option

@PatricKangasmaki
Copy link
Collaborator

Your solution looks good, especially when no additional libraries were needed for this 👍 I think it could be improved a bit if you made a function for the daysAgo -part and moved it to utils folder. It would remove a little bit of code-repeating since the same lines of code are used in two place (getting values for monthAgo in invitations.controller and twoDaysAgo in invitations.model) and maybe even more if you need to use these date-modifiers later in development as well.

Something like this (caution: these are written in this text field for example purposes only, might not work as wanted 😄):

export const daysAgo = (days: number) => {
   const dateNow = new Date();
   return dateNow.setDate(dateNow.getDate() - days);
}

and it could be then used like

.where('updatedAt', '>=', daysAgo(30));
json.valid = json.updatedAt >= daysAgo(2);

Otherwise looking great and ready to go! 🔥

Add timestamp and unique constraint (combination of roadmapId & email)
to invitations. Add route for getting invitations.
@IidaKainu
Copy link
Collaborator Author

Great suggestion, added 👍

@IidaKainu IidaKainu merged commit d9cfe6d into master Oct 31, 2021
@IidaKainu IidaKainu deleted the invitation-backend branch October 31, 2021 08:56
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