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

Feature/cancel reservations #795

Open
wants to merge 74 commits into
base: develop
Choose a base branch
from

Conversation

jlcrodrigues
Copy link
Contributor

@jlcrodrigues jlcrodrigues commented Apr 26, 2023

Closes #656
Depends on #658 , #1073

Added functionality to cancel library reservations.

reservation

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

jlcrodrigues and others added 30 commits November 13, 2022 22:22
@jlcrodrigues jlcrodrigues changed the base branch from feature/library-reservations to develop January 8, 2024 07:45
@jlcrodrigues
Copy link
Contributor Author

I've changed the target and this will now be merged to develop after #1073

Copy link
Member

@bdmendes bdmendes left a comment

Choose a reason for hiding this comment

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

(regarding the failing tests, I'm not sure what's causing the issue since you did not change those files. does it happen on your local machine too?)

Comment on lines 14 to 27
Map<String, String> months = {
'Janeiro': '01',
'Fevereiro': '02',
'Março': '03',
'Abril': '04',
'Maio': '05',
'Junho': '06',
'Julho': '07',
'Agosto': '08',
'Setembro': '09',
'Outubro': '10',
'Novembro': '11',
'Dezembro': '12',
};
Copy link
Member

Choose a reason for hiding this comment

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

Some git issues here


Future<bool> cancelReservation(Session session, String id) async {
final url =
// ignore: lines_longer_than_80_chars
Copy link
Member

Choose a reason for hiding this comment

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

You can split the string, no need for this

Comment on lines +10 to +11
navLibraryOccupation('biblioteca', faculties: {'feup'}),
navLibraryReservations('reservas', faculties: {'feup'}),
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be here? Isn't this on the same page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a different route for each tab. I am using this for each card's onClick

Comment on lines 27 to 28
'/${DrawerItem.navLibraryReservations.title}',
);
Copy link
Member

Choose a reason for hiding this comment

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

Should clicking the card redirect to reservations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it has been fixed

Comment on lines 23 to 27
late final String hoursStart;
late final String hoursEnd;
late final String weekDay;
late final String day;
late final String month;
Copy link
Member

Choose a reason for hiding this comment

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

Prefer to use DateTime and display as you wish below. Also, I don't think this needs to be final

@bdmendes bdmendes requested a review from a team January 12, 2024 13:09
@jlcrodrigues
Copy link
Contributor Author

@bdmendes Fixed the tests and made changes according to your suggestions.

Also, don't forget #1073 is to be merged first.

Comment on lines +11 to +12
// TO DO: Implement parsers for all faculties
// and dispatch for different fetchers
Copy link
Member

Choose a reason for hiding this comment

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

This feature is specific to FEUP, it seems, so is there a need to implement parsers for all faculties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I removed the comment in #1073

"@library_reservations": {},
"library_tab_occupation": "Occupation",
"@library_tab_occupation": {},
"library_tab_reservations": "Reservations",
Copy link
Member

Choose a reason for hiding this comment

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

Can we think of a better translation, maybe? The original one is "Gabinetes".

Copy link
Member

Choose a reason for hiding this comment

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

I just saw that SIGARRA doesn't have a translation for this as well... We can leave it like this!

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.

Cancel reserved rooms
5 participants