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

feat: Add check-in restrictions in settings #1034

Merged
merged 3 commits into from
Jul 23, 2018

Conversation

mooocer
Copy link
Member

@mooocer mooocer commented Jun 8, 2018

Fixes #973

Changes: Adds check-in restrictions preferences for each ticket in Settings

GIF:

videotogif_2018 07 18_01 52 58

@codecov
Copy link

codecov bot commented Jun 8, 2018

Codecov Report

Merging #1034 into development will decrease coverage by 0.52%.
The diff coverage is 0%.

Impacted file tree graph

@@                Coverage Diff                @@
##             development    #1034      +/-   ##
=================================================
- Coverage          27.25%   26.72%   -0.53%     
  Complexity           735      735              
=================================================
  Files                198      202       +4     
  Lines               7588     7738     +150     
  Branches             310      314       +4     
=================================================
  Hits                2068     2068              
- Misses              5434     5584     +150     
  Partials              86       86
Impacted Files Coverage Δ Complexity Δ
...org/fossasia/openevent/app/data/ticket/Ticket.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...event/app/core/settings/EventSettingsFragment.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ttings/restriction/CheckInRestrictionsAdapter.java 0% <0%> (ø) 0 <0> (?)
.../settings/restriction/TicketSettingsViewModel.java 0% <0%> (ø) 0 <0> (?)
...penevent/app/data/ticket/TicketRepositoryImpl.java 73.33% <0%> (-6.88%) 18 <0> (ø)
...core/settings/restriction/CheckInRestrictions.java 0% <0%> (ø) 0 <0> (?)
...sia/openevent/app/core/main/FragmentNavigator.java 40.74% <0%> (ø) 6 <0> (ø) ⬇️
...e/settings/restriction/RestrictionsViewHolder.java 0% <0%> (ø) 0 <0> (?)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd60a42...0d6501c. Read the comment docs.

@mooocer mooocer changed the title [WIP] feat: Add check-in restrictions in settings feat: Add check-in restrictions in settings Jun 20, 2018
@mooocer mooocer force-pushed the check-in branch 2 times, most recently from 38f1c96 to bdcf66a Compare July 17, 2018 20:21
@fossasia fossasia deleted a comment Jul 17, 2018
@@ -51,6 +67,16 @@ public void onCreatePreferencesFix(@Nullable Bundle bundle, String rootKey) {
.commit();
return true;
});


findPreference("check_in_restrictions").setOnPreferenceClickListener(preference -> {
Copy link
Member

Choose a reason for hiding this comment

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

Avoid using string literals


import java.util.List;

public class CheckInRestrAdapter extends RecyclerView.Adapter<CheckInRestrAdapter.TicketViewHolder> {
Copy link
Member

Choose a reason for hiding this comment

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

Use complete name

notifyDataSetChanged();
}

class TicketViewHolder extends RecyclerView.ViewHolder {
Copy link
Member

Choose a reason for hiding this comment

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

Make a separate class, follow principle of separation of concerns.

}

public void updateTicket(Ticket ticket) {

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary line break

}

public void loadTickets() {

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary line break

}

public void updateAllTickets(boolean toRestrict) {

Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -27,4 +28,8 @@
@DELETE("tickets/{id}")
Completable deleteTicket(@Path("id") long id);

@PATCH("tickets/{ticket_id}")
Observable<Ticket> updateTicket(@Path("ticket_id") long id, @Body Ticket ticket);

Copy link
Member

Choose a reason for hiding this comment

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

Extra line break

@@ -9,4 +9,8 @@
android:key="payment_preferences"
android:title="@string/payment_preference"/>

<Preference
android:key="check_in_restrictions"
android:title="Check In Restrictions"/>
Copy link
Member

Choose a reason for hiding this comment

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

Use string resources

@Masquerade0097
Copy link
Member

@MishuVS Why is there option for restricting ticket types which we arn't supporting? Please describe the significance for all the types used.

@mooocer
Copy link
Member Author

mooocer commented Jul 20, 2018

@Masquerade0097

  1. The items listed are individual ticket types and not the ticket categories (Free / Paid / Donation). The items shown are dynamic and not static, and are the tickets created by the organizer for an event.
  2. Providing option to restrict check-in for each ticket makes more sense than to provide option to restrict check-in for ticket categories.
    For example, in the case of having multiple paid tickets like: General Admission, and VIP, when an organizer has a special queue for VIP they can restrict check-in for General Admission - which isn't possible when you restrict ticket category Paid.
  3. This is consistent with the implementation in the EventBrite app.

FragmentTransaction transaction = getFragmentManager().beginTransaction();
transaction
.replace(R.id.fragment_container, ScanSettings.newInstance())
.addToBackStack(null)
.commit();
return true;
});


Copy link
Member

Choose a reason for hiding this comment

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

Extra line


<View
android:background="@color/grey_400"
android:layout_height="1dp"
Copy link
Member

Choose a reason for hiding this comment

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

Use dimens resource

android:title="@string/payment_preference"/>

<Preference
android:key="@string/check_in_restrictions_key"
android:title="Check In Restrictions"/>
Copy link
Member

Choose a reason for hiding this comment

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

String resource



findPreference(getString(R.string.check_in_restrictions_key)).setOnPreferenceClickListener(preference -> {
FragmentTransaction transaction = getFragmentManager().beginTransaction();
Copy link
Member

@Masquerade0097 Masquerade0097 Jul 21, 2018

Choose a reason for hiding this comment

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

I think Transaction should not be needed.

@@ -43,14 +59,24 @@ public void onCreatePreferencesFix(@Nullable Bundle bundle, String rootKey) {
return true;
});

findPreference("scan_settings").setOnPreferenceClickListener(preference -> {
findPreference(getString(R.string.scan_settings_key)).setOnPreferenceClickListener(preference -> {
FragmentTransaction transaction = getFragmentManager().beginTransaction();
Copy link
Member

Choose a reason for hiding this comment

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

same


context = getContext();
if (getArguments() != null)
eventId = getArguments().getLong(MainActivity.EVENT_KEY);
Copy link
Member

Choose a reason for hiding this comment

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

Use brackets

super.onCreate(savedInstanceState);

if (getArguments() != null)
eventId = getArguments().getLong(MainActivity.EVENT_KEY);
Copy link
Member

Choose a reason for hiding this comment

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

Use brackets

private void checkRestrictAll() {
boolean restrictAll = true;

for (Ticket ticket : ticketSettingsViewModel.getTickets().getValue()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can throw NPE ?

}

public void loadTickets() {
compositeDisposable.add(ticketRepository.getTickets(eventId, true)
Copy link
Member

Choose a reason for hiding this comment

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

Pass eventId as argument, will be required for testing.

android:layout_width="0dp"
android:layout_weight="1"
android:layout_height="wrap_content"
android:text='@string/restrict_all'/>
Copy link
Member

Choose a reason for hiding this comment

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

Change to default text color and size.

Copy link
Member Author

@mooocer mooocer Jul 21, 2018

Choose a reason for hiding this comment

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

There's no default style defined right now. Styles should be uniformly handled in other issue.

Copy link
Member

@Masquerade0097 Masquerade0097 Jul 22, 2018

Choose a reason for hiding this comment

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

No, I didn't mean that you define some style. I meant that keep the text color as black and the text size similar to what we keep throughout the app. As per the gif, this screen is looking different with text size small and color grey.

@fossasia fossasia deleted a comment Jul 21, 2018
@mooocer
Copy link
Member Author

mooocer commented Jul 21, 2018

@Masquerade0097 @iamareebjamal Please review

@mooocer mooocer force-pushed the check-in branch 2 times, most recently from b5f4959 to a530953 Compare July 22, 2018 06:02
@fossasia fossasia deleted a comment Jul 22, 2018
Copy link
Member

@Masquerade0097 Masquerade0097 left a comment

Choose a reason for hiding this comment

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

Please resolve the style issue other than that LGTM

@mooocer
Copy link
Member Author

mooocer commented Jul 22, 2018

@iamareebjamal Please review

@fossasia fossasia deleted a comment Jul 22, 2018
@Masquerade0097 Masquerade0097 merged commit 2b0a816 into fossasia:development Jul 23, 2018
@mooocer mooocer deleted the check-in branch July 23, 2018 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants