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

Fixing Workshop issues: Linking any Space & Chat, unlinking and tasks creation #2384

Merged
merged 9 commits into from
Nov 26, 2024

Conversation

gnunicorn
Copy link
Contributor

While running the workshop the other day, we noticed some smaller bugs when going through the flow. This PR fixes them:

  1. Fixes unlinking of chats wasn't working
  2. You get prompted about activating the Tasks feature like with all other features, too
ask-to-activate-tasks-too.mp4
  1. You can now link any Chat or any space as a "sub" one and we cleverly figure out whether that is "recommended" or "sub". This finishes the clean up of "recommended" vs "sub" and removes a bunch of artificial distinction between them for the UI, too
Linking-fixes.mp4

@gnunicorn gnunicorn added bug Something isn't working flutter labels Nov 24, 2024
@@ -20,7 +20,7 @@ Future<void> unlinkChildRoom(
await space.removeChildRoom(roomId, reason);
//Fetch selected room data and add given parentSpaceId as parent
final room = await ref.read(maybeRoomProvider(roomId).future);
if (room != null && room.isSpace()) {
if (room != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what prevented the previous code from properly unlinking chats.

Copy link

codecov bot commented Nov 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.89%. Comparing base (d8d6bdf) to head (53f9ef7).
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2384      +/-   ##
==========================================
+ Coverage   27.34%   27.89%   +0.55%     
==========================================
  Files         604      607       +3     
  Lines       41888    41886       -2     
==========================================
+ Hits        11455    11685     +230     
+ Misses      30433    30201     -232     
Flag Coverage Δ
integration-test 36.88% <ø> (ø)
unittest 20.00% <ø> (+0.79%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -0,0 +1,205 @@
import 'package:acter/common/extensions/options.dart';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the core structure of these has remained the same, but they appears a new files because I moved the content into its own feature and split it up into several distinct widgets that I can also easier test on their own.

class _LinkRoomPageConsumerState extends ConsumerState<LinkRoomPage> {
final TextEditingController searchTextEditingController =
TextEditingController();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed a bunch of stateful ness here that could be done with providers instead. Later realized that we can even collapse most of these providers.

// ChildRoomType configures the sub child type of the `Spaces`
enum ChildRoomType {
chat,
space,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the "recommneded" type as we consider all of them the same, it is really just a question of which listing provider we are using.

Comment on lines +130 to +152
final room = await ref.read(maybeRoomProvider(roomId).future);
if (room == null) {
EasyLoading.showError(lang.roomNotFound);
return;
}

final canLink = (await ref.read(roomMembershipProvider(roomId).future))
?.canString('CanLinkSpaces') ==
true;

//Fetch selected parent space data and add given roomId as child
final space = await ref.read(spaceProvider(parentId).future);
await space.addChildRoom(roomId, false);

//Make subspace
if (canLink) {
//Fetch selected room data and add given parentSpaceId as parent
await room.addParentRoom(parentId, true);
if (!context.mounted) return;
await checkJoinRule(context, ref, room, parentId);
} else {
EasyLoading.showSuccess(lang.roomLinkedButNotUpgraded);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now accepts both the non upgradable and the uprgadable cases and only shows the dialog if you can upgrade. If you can't uprgade you'll get a success messages with a note instead.

iconData: Atlas.list,
title: lang.addTask,
onPressed: () async {
if (!canAddEvent && canChangeSetting) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the actual bug. canAddEvent is the wrong check here. However, while looking at it, I also noticed that we are only checking the permission but not if the feature is activated, so I did refactor the entire file to check for the feature-activation more specifically.

@@ -0,0 +1,147 @@
import 'package:acter/common/providers/chat_providers.dart';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yay tests \o/ !

);
}

Future<void> checkJoinRule(
Copy link
Contributor

@gtalha07 gtalha07 Nov 26, 2024

Choose a reason for hiding this comment

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

This can be moved over created actions folder along with other methods

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 was thinking about it but consider this very specific to this flow at the moment and rather not have this be reusable for others yet ...

Copy link
Contributor

@gtalha07 gtalha07 left a comment

Choose a reason for hiding this comment

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

Looks good!

@gnunicorn gnunicorn merged commit 4e0dacf into main Nov 26, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working flutter
Projects
Status: Recently Done
Development

Successfully merging this pull request may close these issues.

2 participants